[songgaoye]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

SelectiveChannel::CheckHealth() and verbose Describe() invoke sub-channel
methods while holding the channel map mutex. Slow sub-channel operations can
therefore delay concurrent add and removal operations.

What is changed and the side effects?

Changed:

• Use a weakly consistent snapshot for OWNS_CHANNEL registrations, resolving
  and accessing them after releasing the channel map mutex.
• Keep synchronization for DOESNT_OWN_CHANNEL registrations so that removal
  cannot return and allow the caller to destroy an externally owned channel
  while it is being accessed.

Side effects:

• CheckHealth() and verbose Describe() may observe a weakly consistent view
  of owning registrations during concurrent add or removal operations.

• Non-owning registrations retain the previous synchronization behavior to
  preserve their lifetime contract.

• Performance effects:

  • Reduces mutex contention and improves tail latency for concurrent add and
    removal operations when health checks or verbose descriptions of owning
    sub-channels are slow.
  • No lock-granularity improvement applies to non-owning registrations.

• Breaking backward compatibility:

  • None.

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

5 open issues

• static_cast<SubChannel*>(ptr->user())->chan dereferences ptr->user() without validating it is… · New
• Same issue as in CheckHealth(): ptr->user() is dereferenced without null/type validation. If… · New
• Same issue as in CheckHealth(): ptr->user() is dereferenced without null/type validation. If… · New
• ASSERT_TRUE(blocking->started()) can cause the test process to hang: if this assertion fails,… · New
• The AddChannel() completion wait is capped at ~100ms, which is prone to CI flakiness under… · New

What changed in this PR

This PR reduces lock contention in SelectiveChannel by snapshotting sub-channel SocketIds under ChannelBalancer::_mutex, then releasing the lock before running potentially slow CheckHealth() / verbose Describe() on each sub-channel. It also adds a regression test to ensure verbose Describe() doesn’t block AddChannel().

Changes:

• Snapshot sub-channel SocketIds while holding ChannelBalancer::_mutex, then resolve/access sockets after unlocking.
• Update CheckHealth() and verbose Describe() to use Socket::AddressFailedAsWell() + SocketUniquePtr for safe access.
• Add a unit test ensuring verbose Describe() doesn’t block AddChannel().

File	Description
test/​brpc_channel_unittest.cpp	Adds a regression test with a blocking sub-channel Describe() to verify AddChannel() is not blocked by verbose Describe().
src/​brpc/​selective_channel.cpp	Reworks CheckHealth() / verbose Describe() to avoid holding _mutex while invoking sub-channel operations by using snapshot-based iteration.

_{Review effort: Lite}

[Copilot]

🟡 Changes recommended

Concurrent removal can leave unlocked readers accessing deleted, externally owned sub-channels.

2 open issues

• Protect externally owned channels during health checks and descriptions · New
• The AddChannel() completion wait is capped at ~100ms, which is prone to CI flakiness under…

4 resolved since last review

• ASSERT_TRUE(blocking->started()) can cause the test process to hang: if this assertion fails,…
• Same issue as in CheckHealth(): ptr->user() is dereferenced without null/type validation. If…
• Same issue as in CheckHealth(): ptr->user() is dereferenced without null/type validation. If…
• static_cast<SubChannel*>(ptr->user())->chan dereferences ptr->user() without validating it is…

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The independently changed health-check locking and lifetime behavior needs blocking concurrency regression tests.

1 open issue

• Add blocking health-check concurrency and lifetime tests · New

2 resolved since last review

• Protect externally owned channels during health checks and descriptions
• The AddChannel() completion wait is capped at ~100ms, which is prone to CI flakiness under…

_{Review effort: Balanced}

[wwbmmm]

I don't think this is necessary.