search

Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.25k stars 4.59k forks source link

Add convenience helper to create RequestContext from CancellationToken #29015

Closed annelo-msft closed 3 months ago

annelo-msft commented 2 years ago

For the DPG grow-up story, we would like a way to convert a CancellationToken passed into a convenience method into a RequestContext to pass into the protocol method. We would like to standardize this using an approach that does minimal allocations.

@tg-msft has suggested an implementation such as this one: https://github.com/Azure/azure-sdk-for-net/blob/5850bfe18b837d9c6108730f72f339c3a4cb8e03/sdk/core/Azure.Core/src/Shared/RequestContextExtensions.cs#L27-L30, but @KrzysztofCwalina has raised concerns about the possibility of callers modifying the default.

This issue tracks the work to close on the design and add the helper to Azure.Core.

pshao25 commented 2 years ago

about the possibility of callers modifying the default.

Could you help me understand what the default refers to?

And seems this solution will anyhow initialize a new instance of RequestContext (Empty will create a new instance too). Could you explain how it could reduce the allocation? https://github.com/Azure/azure-sdk-for-net/blob/5850bfe18b837d9c6108730f72f339c3a4cb8e03/sdk/core/Azure.Core/src/Shared/RequestContextExtensions.cs#L18

annelo-msft commented 2 years ago

And seems this solution will anyhow initialize a new instance of RequestContext (Empty will create a new instance too). Could you explain how it could reduce the allocation?

It will initialize the value of Empty once. Since it is a static property, that value will be the same across the process whenever someone calls RequestContext.Empty. So, it can be reused without making any new allocations.

"the default" in this case means the value of Empty. If anyone were to modify this accidentally, all future calls to RequestContext.Empty would get a modified version, which is not what they are expecting.

KrzysztofCwalina commented 2 years ago

One way to address the issue would be to generate internal static property (on the client for example) to store the singleton RequestContext, instead of having a public RequestContent.Empty property.

tg-msft commented 2 years ago

One way to address the issue would be to generate internal static property (on the client for example) to store the singleton RequestContext, instead of having a public RequestContent.Empty property.

The proposal here is an internal static shared source property which is 95% of what you're suggesting. The only difference I see is it's shared across all the clients in a library rather than per client.

github-actions[bot] commented 3 months ago

Hi @annelo-msft, we deeply appreciate your input into this project. Regrettably, this issue has remained unresolved for over 2 years and inactive for 30 days, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.