Make ServicePartitionClient recreate TCommunicationClient based on ServiceNotification

[olegsych]

ServicePartitionClient keeps reusing the TCommunicationClient created by the ICommunicationClientFactory until it's ReportOperationExceptionAsync method returns OperationRetryControl.IsTransient=false. While the current design trusts the ICommunicationClientFactory to determine if the client is still valid, implementing this logic may not be trivial and mistakes can lead to prolonged attempts to use a permanently broken TCommunicationChannel.

We should change the ServicePartitionClient so that it always uses the client returned by the ICommunicationClientFactory.ICommunicationClientFactory.GetClientAsync. The term Get in this API indicates that the factory is responsible for caching the clients, not just creating them. The current implementation of the CommunicationClientFactoryBase would ensure that the endpoint information of the partition would be checked against the current information in the FabricClient and even if the user code didn't detect the endpoint change breaking the client, the next attempt to communicate with this partition it would create a fresh client.

We should also make the CommunicationClientFactoryBase handle the FabricClient.ServiceManagementClient.ServiceNotificationFilterMatched event and re-create recently-used cached clients in the background, so that the next GetClientAsync request is likely to find a new client in the cache and avoid the delay of trying to use a broken client, potentially waiting until it times out, creating a new client and opening the channel.

https://dev.azure.com/msazure/One/_workitems/edit/37022576

[olegsych]

❓ Does this change expectations for thread safety of the TCommunicationClient instances?

[ilijagrbic]

Hey @olegsych , I have couple of thoughts I would like to share on this.

A simpler alternative worth considering is periodic cache invalidation for cached TCommunicationClient instances.
The core problem seems to be that once a client becomes permanently broken, the only way to get a new client instance is by exception that is thrown to be IsTransient = false (or to recreate everything by doing a failover of the client service). Periodically invalidating cached clients (for example by associating a max‑age with each client instance) would put a hard upper bound on how long a broken client can live.

This approach seems to be supported in the code already. For example, here we check if the cache entry is valid and if we need to recreate it. Right now, that logic is limited to a simple non-null check but can easily be expanded to check for the max-age. Another pro for this approach is that we can localize changes to one method, and relatively easily unit test this.

[olegsych]

I like the idea of limiting maximum lifespan of a client - it would allow us to limit the outage time even if the service notification doesn't work as expected. I think I've used lifespan of ~15 minutes in my own services. But even at this rate, it would create measurable impact on client availability, so it must be combined with the wait-free recreation of recently used clients in the background.

[olegsych]

I think we now have a cache implementation we can actually use in services. Unfortunately, it's a Microsoft.Extensions package, but the one in System can't be used directly at all.

https://learn.microsoft.com/en-us/dotnet/core/extensions/caching