Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ServiceClient so ProductInfoHeaderValues are not duplicated and are merged without error #4095

Closed
wants to merge 5 commits into from
Closed

Update ServiceClient so ProductInfoHeaderValues are not duplicated and are merged without error #4095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
611dd42
ServiceClient updates
Feb 26, 2018
6f95e66
Update CustomClientWithHttpClientTests
Feb 26, 2018
e133132
Update ServiceClient.cs
Feb 27, 2018
46ae177
Dispose root handler in constructors that create it
Mar 6, 2018
d798fae
Merge branch 'psSdkJson6' into psSdkJson6
shahabhijeet Mar 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/SdkCommon/ClientRuntime/ClientRuntime.Tests/CustomClientWithHttpClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,48 @@ public void AddFxVersionHeaderInformation()
Assert.Equal(defaultVersion.ToString(), "1.0.0.0");
}

/// <summary>
/// This is to verify if a HttpClient is passed, we add default userAgent information
/// inside defaultheaders of the passed in HttpClient ONLY once
/// </summary>
[Fact]
public void AddFxVersionHeaderInformationOnce()
{
//Version defaultVersion = null;
HttpClient hc = new HttpClient(new ContosoMessageHandler());
ContosoServiceClient contosoServiceClient = new ContosoServiceClient();
string productName = contosoServiceClient.GetType().FullName;
contosoServiceClient.Dispose();
hc.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue(productName, "1.0.0.0"));

contosoServiceClient = new ContosoServiceClient(hc, false);
HttpResponseMessage response = contosoServiceClient.DoSyncWork();
contosoServiceClient.Dispose();

HttpHeaderValueCollection<ProductInfoHeaderValue> userAgentValueCollection = contosoServiceClient.HttpClient.DefaultRequestHeaders.UserAgent;
var productInfos = userAgentValueCollection.Where<ProductInfoHeaderValue>((p) => p.Product.Name.Equals(productName, StringComparison.OrdinalIgnoreCase));
Assert.Single(productInfos);
}

/// <summary>
/// This is to verify if a HttpClient is passed that contains ProductInfoHeaderValues with comments in
/// userAgent information, no exception occurs while merging
/// </summary>
[Fact]
public void IgnoreProductInfoHeaderValueCommentsDuringMerge()
{
const string comment = "(comment)";
HttpClient hc = new HttpClient(new ContosoMessageHandler());
hc.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue(comment));
ContosoServiceClient contosoClient = new ContosoServiceClient(hc, false);
HttpResponseMessage response = contosoClient.DoSyncWork();

HttpHeaderValueCollection<ProductInfoHeaderValue> userAgentValueCollection = contosoClient.HttpClient.DefaultRequestHeaders.UserAgent;
contosoClient.Dispose();
var productInfos = userAgentValueCollection.Where<ProductInfoHeaderValue>((p) => p.Product == null && p.Comment.Equals(comment, StringComparison.OrdinalIgnoreCase));
Assert.Single(productInfos);
}

private HttpResponseMessage SendAndReceiveResponse(HttpClient httpClient)
{
// Create HTTP transport objects
Expand Down
29 changes: 19 additions & 10 deletions src/SdkCommon/ClientRuntime/ClientRuntime/ServiceClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.

namespace Microsoft.Rest
Expand Down Expand Up @@ -44,6 +44,11 @@ public abstract class ServiceClient<T> : IDisposable
/// </summary>
private bool _disposeHttpClient;

/// <summary>
/// Flag to track if the root handler needs to disposed
/// </summary>
private bool _disposeRootHandler;

/// <summary>
/// Field used for ClientVersion property
/// </summary>
Expand Down Expand Up @@ -245,6 +250,7 @@ private string FrameworkVersion
protected ServiceClient()
: this(CreateRootHandler())
{
_disposeRootHandler = true;
}

/// <summary>
Expand Down Expand Up @@ -273,6 +279,7 @@ protected ServiceClient(HttpClient httpClient, bool disposeHttpClient = true)
protected ServiceClient(params DelegatingHandler[] handlers)
: this(CreateRootHandler(), handlers)
{
_disposeRootHandler = true;
}

/// <summary>
Expand Down Expand Up @@ -384,8 +391,11 @@ protected virtual void Dispose(bool disposing)
{
HttpClient.Dispose();
HttpClient = null;
}

}

if (_disposeRootHandler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone who uses this constructor:

https://github.com/EricDahlvang/azure-sdk-for-net/blob/e13313279e108e350289dc7373414bcac4e4602a/src/SdkCommon/ClientRuntime/ClientRuntime/ServiceClient.cs#L288

        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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I missed something. Constructors on lines 250 and 258 also create the RootHandler, and in those cases it should be disposed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

HttpClientHandler.Dispose();

FirstMessageHandler = null;
HttpClientHandler = null;
}
Expand Down Expand Up @@ -419,6 +429,8 @@ protected void InitializeHttpClient(HttpClient httpClient, HttpClientHandler htt
if(httpClientHandler == null)
{
httpClientHandler = CreateRootHandler();
//set flag to dispose of handler, since it was not supplied and had to be created here
_disposeRootHandler = true;
}

HttpClientHandler = httpClientHandler;
Expand Down Expand Up @@ -471,9 +483,8 @@ public bool SetUserAgent(string productName, string version)
{
if (!_disposed && HttpClient != null)
{
MergeUserAgentInfo(DefaultUserAgentInfoList);
string cleanedProductName = CleanUserAgentInfoEntry(productName);
HttpClient.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue(cleanedProductName, version));
string cleanedProductName = CleanUserAgentInfoEntry(productName);
MergeUserAgentInfo(DefaultUserAgentInfoList.Concat( new[] { new ProductInfoHeaderValue(cleanedProductName, version) }));
return true;
}

Expand All @@ -499,13 +510,11 @@ private string CleanUserAgentInfoEntry(string infoEntry)
/// We do this because, now we accept passed in HttpClient.
/// So for any reason the passed HttpClient has our default UserAgent info (based on key name), we will not verify and check the values and will honor those values
/// </summary>
private void MergeUserAgentInfo(List<ProductInfoHeaderValue> defaultUserAgentInfoList)
private void MergeUserAgentInfo(IEnumerable<ProductInfoHeaderValue> defaultUserAgentInfoList)
{
// If you want to log ProductName in userAgent, it has to be without spaces

foreach(ProductInfoHeaderValue piHv in defaultUserAgentInfoList)
{
if(!HttpClient.DefaultRequestHeaders.UserAgent.Any<ProductInfoHeaderValue>((hv) => hv.Product.Name.Equals(piHv.Product.Name, StringComparison.OrdinalIgnoreCase)));
if (!HttpClient.DefaultRequestHeaders.UserAgent.Any<ProductInfoHeaderValue>((hv) => hv.Product != null && hv.Product.Name.Equals(piHv.Product.Name, StringComparison.OrdinalIgnoreCase)))
{
HttpClient.DefaultRequestHeaders.UserAgent.Add(piHv);
}
Expand Down