Update ServiceClient so ProductInfoHeaderValues are not duplicated and are merged without error #4095
Conversation
Check that .Product is not null during merge of ProductInfoHeaderValues. Do not add Product to UserAgent if already present. Dispose of root handler if created.
Add tests to ensure ProductInfoHeaderValues are not added to UserAgent twice, and those constructed with 'comment' constructor are ignored during merging.
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
ref: #4086 |
|
|
||
| } | ||
|
|
||
| if (_disposeRootHandler) |
There was a problem hiding this comment.
@EricDahlvang let's just call Dispose on the set the Handlers that are part of ServiceClient. I don't think it's required to have a flag to track it.
There was a problem hiding this comment.
If the ServiceClient did not create the handler, then it should not dispose it. This addition only disposes it if it was internally created.
issue ref: #3712
There was a problem hiding this comment.
@EricDahlvang Maybe I am not able to understand your scenario.
Can you tell me a scenario where someone will initialize Service client and would like to use it's RootHandler after disposing the service client.
There was a problem hiding this comment.
Anyone who uses this constructor:
protected ServiceClient(HttpClientHandler rootHandler, params DelegatingHandler[] handlers)
{
InitializeHttpClient(rootHandler, handlers);
}
(I don't personally use that constructor, but I saw that this is an issue ... so included it in this PR)
There was a problem hiding this comment.
It looks like I missed something. Constructors on lines 250 and 258 also create the RootHandler, and in those cases it should be disposed.
There was a problem hiding this comment.
@EricDahlvang is this still in discussion, I found some issue related.I agree with you.It's not thread safe when set the user agent to the http client header.This will lead some critical issue when using http client
There was a problem hiding this comment.
@mjaow I have no idea if this is still in discussion. The azure sdk team's previous discussion with me was not fruitful. I'm not actually on the azure sdk team, but was just trying to help. The team apparently wanted to make the changes themselves. Things just got worse imo.
- As far as I know, ProductInfoHeaderValues without a product (comment only) will still throw exceptions in this code:
https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/mgmtcommon/ClientRuntime/ClientRuntime/ServiceClient.cs#L567
if (!HttpClient.DefaultRequestHeaders.UserAgent.Contains<ProductInfoHeaderValue>(pInfoHeaderValue,
new ObjectComparer<ProductInfoHeaderValue>((left, right) => left.Product.Name.Equals(right.Product.Name, StringComparison.OrdinalIgnoreCase))))
{
...-
Created objects are still not properly disposed.
-
The locking strategy in ServiceClient does not make sense to me, and I think ServiceClient should just be a singleton per service.
There was a problem hiding this comment.
@EricDahlvang , yeah I'm in the same situation with you.I need to create ContainerInstanceManagementClient every request due to the different subscription id and token info.Creating a new ContainerInstanceManagementClient instance will create a new ServiceClient. So the lock seems useless in this condition
I think the right way to fix is to remove the part of setting user agent
| string cleanedProductName = CleanUserAgentInfoEntry(productName); | ||
| HttpClient.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue(cleanedProductName, version)); | ||
| string cleanedProductName = CleanUserAgentInfoEntry(productName); | ||
| MergeUserAgentInfo(new ProductInfoHeaderValue(cleanedProductName, version)); |
There was a problem hiding this comment.
What we really need is the deDuping of UserAgent collection.
Yes this unblocks you for the nullRef, but this has perf. impact and I would prefer a deDuping of UserAgent collection than what is implemented in your PR.
So if you can update your PR with 1 time deDuping of UserAgent collection that will be great, else we can do that.
There was a problem hiding this comment.
I've made the requested changes.
Frankly, this method for deDuping UserAgent isn't enough for my use case. I'd like to use the constructor that allows me to pass in an HttpClient. However, I want to use a Static HttpClient ... and this code is not threadsafe. I believe a better option would be to not add UserAgent headers at all, if the consumer has provided their own HttpClient. What are your thoughts?
|
@azuresdkci retest this please |
|
Is there something holding up this PR being merged? |
|
Can one of the admins verify this patch? |
|
I totally understand having difficulty with a PR someone submits, and you do not want to accept. If you have a different plan to handle the underlying issues I've attempted to address with this, please let me know. We can just close this PR without merging, and resolve these issues some other way if that is the preference of your team. From my perspective, I don't see any reason to just leave this sitting here without merging, without feedback and without discussion. I will understand if you want to just close this, but please address the underlying issues. |
|
@EricDahlvang opened a new PR that addresses multiple issues including avoiding duplicating userAgent strings. |
Description
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.