Fix NettyChannelTracker race condition when tracking new channels

[injectives]

This update fixes an issue when SimpleChannelPool may invoke the channelAcquired method before the channelCreated method is called. Since it violates NettyChannelTracker contract, it may result in runtime failures.

See the following for details:

@startuml
T1 -> SimpleChannelPool: acquireHealthyFromPoolOrNew
activate SimpleChannelPool

SimpleChannelPool -> NettyChannelPool: connectChannel
activate NettyChannelPool

NettyChannelPool -> ChannelFuture: addListener
activate ChannelFuture

ChannelFuture --> NettyChannelPool:
deactivate ChannelFuture

NettyChannelPool --> SimpleChannelPool:
deactivate NettyChannelPool

T2 -> ChannelFuture: setSuccess
activate ChannelFuture

ChannelFuture -> ChannelFuture: done
activate ChannelFuture

ChannelFuture --> ChannelFuture:
deactivate ChannelFuture

SimpleChannelPool -> ChannelFuture: isDone
activate ChannelFuture

ChannelFuture --> SimpleChannelPool:
deactivate ChannelFuture

alt isDone case
    SimpleChannelPool -> NettyChannelTracker: channelAcquired
    activate NettyChannelTracker

    NettyChannelTracker --> SimpleChannelPool:
    deactivate NettyChannelTracker
end

SimpleChannelPool --> T1:
deactivate SimpleChannelPool

ChannelFuture -> NettyChannelTracker: channelCreated
activate NettyChannelTracker

NettyChannelTracker -> ChannelFuture:
deactivate NettyChannelTracker

ChannelFuture -> T2:
deactivate ChannelFuture
@enduml

[michael-simons]

http://www.plantuml.com/plantuml/png/ZL91JiCm4BplA_o17jfZ3wYegEA02bBp0SixKYCS1-mrgBydeL6ao8j9JztDpEpCUWz9R9JSUMgsPlDWBcttuN6ysX3Wt_hUxurrdxcDU8RrShsTOj-D_TTunZTP9-sN5PGyeg8rwfyXSbl8ktxuxsJgpOe5acZHcVIcyfGbHmnUcL_Q90Y8iz8IGxGepKOpHGoThPbGjqQCIf9cftXFa4js3Ycjiwv8S1-mbbomAw6BVChiDZsjdrvPcawtNgO9nja4CiFltmDhedNlY7lZxlNZ_PBvbprFKXWq8hGFVnaVuOog909hYnm5c-qYUW5I7huMynXX1KprX6gwsaqe1CLOiwiyum61S-T_0000

[michael-simons]

Since it violates NettyChannelTracker contract, it may result in runtime failures.

just to be sure, this is about the old situation?

[michael-simons]

Looks good to me.

A couple of things:

• I wonder if we can reproduce the failures locally when metrics are turned on
• did we actually make the failures happen more often because the tracker is now thread safe after the previous fix attempts?
• If this works I like it to be backported to 4.2 and 4.1 but I can do that as well.

[injectives]

Since it violates NettyChannelTracker contract, it may result in runtime failures.

just to be sure, this is about the old situation?

This is about the case that this change tries to fix.

[injectives]

• I wonder if we can reproduce the failures locally when metrics are turned on

I tried setting up a simple case on local environment, but could not reproduce this. Given that this is effectively a race condition, it might be tricky to reproduce. Fortunately, we have some CI builds that may help us with this.

[injectives]

• did we actually make the failures happen more often because the tracker is now thread safe after the previous fix attempts?

This change did not seem to make a negative impact on my local environment, it will be verified further in CI builds.

[injectives]

• If this works I like it to be backported to 4.2 and 4.1 but I can do that as well.

Since it fixes a race condition that may occur, it makes sense to backport it.

[injectives]

Looks good to me.

Thanks 👍