Azure-sdk-for-net: Document suggested lifetime patterns for clients

[fschmied]

This is the same issue as https://github.com/Azure/azure-storage-net/issues/732, but for the new SDK.

The docs (e.g., https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Blobs/README.md, https://docs.microsoft.com/en-us/dotnet/api/azure.storage.blobs.blobserviceclient?view=azure-dotnet, https://docs.microsoft.com/en-US/azure/storage/blobs/storage-quickstart-blobs-dotnet, or https://docs.microsoft.com/en-US/azure/storage/blobs/storage-blobs-introduction) don't mention anything about the thread safety or suggested lifetime/usage patterns of BlobServiceClient, BlobContainerClient, and BlobClient.

Are you supposed to create one instance of these per application, web request, or storage access operation? Will the wrong pattern create resource exhaustion? Or doesn't it matter?

Please document this.

[fschmied]

This is a very good writeup of the problem on the other issue by @AGBrown: https://github.com/Azure/azure-storage-net/issues/732#issuecomment-554336356

[msftbot[bot]]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage

[msftbot[bot]]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage

[AlexGhiondea]

/cc @tg-msft

[tg-msft]

We have a guideline that all client methods must be thread-safe:

DO be thread-safe. All public members of the client type must be safe to call from multiple threads concurrently.

The only exceptions are stateful model objects (including tricky models like BlobBatch that look a lot like clients but never make service requests themselves). We didn't want to synchronize all model mutations so users should handle that themselves if sharing models across threads.

+@pakrym to comment more about lifetime, but we share a single HttpClient instance across all of the clients in your application (by default if you're not customizing the Transport) so you're generally only paying for the allocations.

[pakrym]

Clients are supposed to be created once per application, currently there is no reason to create more of them.

As @tg-msft pointed out we cache the default HttpClient instance we use so creating multiple client instances would just result in allocations but not in any HttpClient related problems.

[fschmied]

Clients are supposed to be created once per application, currently there is no reason to create more of them.

[...] we cache the default HttpClient instance we use so creating multiple client instances would just result in allocations but not in any HttpClient related problems.

Thank you for the very clear answer. It would be great if you could add these two sentences to the docs, ideally both on the concept pages and on the API docs for each of the affected clients.

[msftbot[bot]]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc

[tg-msft]

I think this is a good suggestion and we should create a standard blurb for the <remarks> section of our client documentation.

We should also probably have a Best Practices sample that talks about these things in more detail _(for both Core and Storage)_ that can grow over time. There are additional subtleties worth calling out like starting with a BlobServiceClient and navigating down the hierarchy via Get***Client() methods rather than constructing new BlobContainerClient or BlobClient instances directly _(that'll avoid recreating the transport pipeline - which won't cause resource issues, but isn't a free computation)_.

[paul-lorica]

+1

[Petermarcu]

Removing Storage label because we need this for all clients. I'm going to mark it as Core because of that.

[fiksen99]

@pakrym

Clients are supposed to be created once per application, currently there is no reason to create more of them.

I challenge this - consider a scenario where our service receives a request that contains a reference to some arbitrary and transient Azure Storage container with a SAS URI. Currently the BlobClient can be constructed with a SasUri, which would require a new client per request. Is this feasible in the current BlobClient design?

[pakrym]

Yes, you are right. I should've said "Clients are supposed to be created once per application for a given set of client parameters". If you connect to different endpoints or use different credentials you would definitely need more than one instance of a client type but only one for a given set of parameters.

[fiksen99]

Great, thanks for confirming. We are following this pattern in our service, and given the nature of this thread, I also wanted to confirm that my takeaway that this shouldn't be a problem for resource utilization (e.g. outbound connections) was correct.

[pakrym]

We have a blog post on this topic https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients/

[PureKrome]

@pakrym Great post! Will it be 'linked' to any documentation in the official docs website (so in the future, it might be easy to find)

[pakrym]

Yes, I think we still need to turn into an official doc somewhere.

[tg-msft]

@pakrym - would you mind copying that blog post to https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/core/Azure.Core/samples as a start? We should also get a version of that blog post included in the conceptual docs and I want to update our implementation guidelines to suggest standard blurbs about thread-safety, lifetime, etc., so all of this appears in the Remarks section of our reference documentation for all client libraries.

[tg-msft]

I mean standard doc comment blurbs. :smile: I want something easily copy/pasted (and maybe eventually verified) so we have consistent looking ref docs that cover all these details new developers wonder about but we don't necessarily consider when writing our n-th client library.

[jongio]

@pakrym - It would be great to try to close this one out as it is 264 days old as of today. Thanks, Jon