Revert "services: fix HealthCheckingLoadBalancer.shutdown(). (#5848)"#5875
Revert "services: fix HealthCheckingLoadBalancer.shutdown(). (#5848)"#5875ejona86 merged 1 commit intogrpc:masterfrom
Conversation
…)" This reverts commit c6f1516. It broke an internal health checking test because the server wouldn't shut down. We assume the health checking RPC isn't getting closed.
|
The reason it doesn't work is because Currently we have the restriction that PRs are on the way. |
|
@zhangkun83 Could you fix this soon, because our internal lb interop tests with java massage client is flaky |
…dBalancer is shutdown. (#5883) No more methods on the `LoadBalancer` will be called after `LoadBalancer#shutdown()` is called. This includes `LoadBalancer#handleSubchannelState()` too. `SubchannelStateListener` inherited this restriction. However, this special case makes `onSubchannelState(SHUTDOWN)` an unreliable way of being notified about `Subchannel` SHUTDOWN, and may confuse/complicate a wrapping `LoadBalancer` that expects the full notification (e.g., #5875). The javadoc isn't clear whether this restriction applies. I think it's more useful to make it no apply.
The issue: HealthCheckingLoadBalancer.shutdown() calls hcState.onSubchannelState(SHUTDOWN) which removes that hcState from helper.hcStates. Therefore, if more than one Subchannels are present, ConcurrentModificationException will be thrown. This is an alternative approach from #5848 that was reverted in #5875. Thanks to #5883, HealthCheckingLoadBalancer.shutdown() no longer has to fake SHUTDOWN notifications, and can completely rely on Subchannels' real SHUTDOWN notifications for triggering the clean-up.
This reverts commit c6f1516 (#5848). It broke
an internal health checking test because the server wouldn't shut down.
We assume the health checking RPC isn't getting closed.