Simplify ClientConnection implementation

[bernardnormier]

    I still find the `ClientConnection` implementation too complex. 

To me, it should be a "simplified" connection cache implementation. It should keep track of the pending connect (_pendingConnectTask), the active connection (_activeConnection) and the background connection dispose count (_backgroundConnectionDisposeCount). No need for decorators.

Originally posted by @bentoi in #2396 (review)

[bernardnormier]

ClientConnection, unlike ConnectionCache, serializes connections to the server and I believe it is a feature we should keep.

See also my blog: https://zeroc.atlassian.net/wiki/spaces/~bernard/blog/2023/01/02/1211039772/January+2+2023

[bernardnormier]

I think the 2 decorators used for the implementation of ClientConnection are appropriate.

The ConnectProtocolConnectionDecorator makes sure we only InvokeAsync on a connected connection, even when:

• the application calls ClientConnection.ConnectAsync concurrently (which would be dumb but allowed)
• multiple ClientConnection.InvokeAsync are in progress

ConnectionCache is simpler in this respect because it does not need to worry about application code calling ConnectAsync.

The CleanupProtocolConnectionDecorator makes sure we ConnectAsync a connection only have the cleanup of the previous connection is completed. And in case we never call ConnectAsync on that connection, its DisposeAsync will await the cleanup of the previous connection.

ConnectionCache doesn't provide this connection serialization feature.

It's certainly possible to do the same without decorators but I doubt it would be simpler or clearer.

[bentoi]

ClientConnection, unlike ConnectionCache, serializes connections to the server and I believe it is a feature we should keep.

It's not obvious to me why it's a feature we should keep, can you explain why?

[bernardnormier]

The alternative - no serialization like ConnectionCache - would mean:

• a single ClientConnection can generate N concurrent connections to the same Server
• "N - 1" connections are "shutdown requested", shutting down or being disposed
• we'd need to wait for all these shutdowns / disposes somewhere, presumably in ShutdownAsync and DisposeAsync

I think it's cleaner and clearer to serialize.

[bentoi]

I find it's minor and it's still unclear to me why client connection / connection cache should behave differently in this respect.

[bernardnormier]

@bentoi is right, I will synchronize it with ConnectionCache together with #2566.