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.

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

Adding a way to initialize ServiceClient using provided HttpClient #2909

Closed
wants to merge 8 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ public FakeServiceClient()
// Prevent base constructor from executing
}

public FakeServiceClient(HttpClient httpClient)
: base(httpClient)
{ }

public FakeServiceClient(HttpClientHandler httpMessageHandler, params DelegatingHandler[] handlers)
: base(httpMessageHandler, handlers)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Xunit;
using System.Net.Http.Headers;
using System.Diagnostics;
using Microsoft.Rest.ClientRuntime.Tests.SvcClients;

namespace Microsoft.Rest.ClientRuntime.Tests
{
Expand Down Expand Up @@ -109,6 +110,16 @@ public void RetryHandlerRetriesWith500Errors()
Assert.Equal(2, attemptsFailed);
}

[Fact]
public void FakeSvcClientWithHttpClient()
{
HttpClient hc = new HttpClient(new ContosoMessageHandler());
var fakeClient = new FakeServiceClient(hc);
HttpResponseMessage response = fakeClient.DoStuffSync("Hello");
string responseContent = response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult();
Assert.Equal("Contoso Rocks", responseContent);
}

[Fact]
public void RetryHandlerRetriesWith500ErrorsAndSucceeds()
{
Expand Down Expand Up @@ -214,7 +225,7 @@ public void AddUserAgentInfoWithVersion()
Assert.True(testProduct.Product.Name.Equals(testProductName));
Assert.True(testProduct.Product.Version.Equals(testProductVersion));
}

#if NET451
[Fact]
public void VerifyOsInfoInUserAgent()
Expand All @@ -230,6 +241,10 @@ public void VerifyOsInfoInUserAgent()
Assert.NotEmpty(osProduct.Product.Name);
Assert.NotEmpty(osProduct.Product.Version);
}




#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove line breaks from 244 to 248

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
using Microsoft.Rest.ClientRuntime.Tests.SvcClients;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using Xunit;

namespace Microsoft.Rest.ClientRuntime.Tests
{
public class SvcClientWithHttpClientTests
{
[Fact]
public void InitializeServiceClientWithHttpClient()
{
HttpClient hc = new HttpClient();
ContosoServiceClient contosoClient = new ContosoServiceClient(hc);
HttpResponseMessage response = contosoClient.DoSyncWork();
Assert.NotNull(response);
}

[Fact]
public void GetInitializedHttpClient()
{
ContosoServiceClient contosoClient = new ContosoServiceClient(null);
HttpResponseMessage response = contosoClient.DoSyncWork();
string cont = response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult();
Assert.NotNull(response);
}

[Fact]
public void InitializeMessageHandlerPostContructor()
{
HttpClient hc = new HttpClient(new ContosoMessageHandler());
ContosoServiceClient contosoClient = new ContosoServiceClient(hc);
HttpResponseMessage response = contosoClient.DoSyncWork("Hello");
string cont = response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult();
Assert.Equal("Contoso Rocks", cont);
}

[Fact]
public void ProvideHttpClientAfterInitialization()
{
DelegatingHandler[] handlers = new ContosoMessageHandler[] { new ContosoMessageHandler() };
ContosoServiceClient contosoClient = new FabricamServiceClient(new HttpClientHandler(), handlers);
HttpResponseMessage response = contosoClient.DoSyncWork();
string cont = response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult();
Assert.Equal("Delayed User Provided HttpClient after initialization", cont);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.Rest.ClientRuntime.Tests.SvcClients
{
/// <summary>
/// Customized client that emulates how partners will use Customized code to extend
/// generated client.
/// </summary>
public class ContosoServiceClient : ServiceClient<ContosoServiceClient>
{
HttpClient _httpClient;
/// <summary>
/// Constructor that accepts HttpClient
/// </summary>
/// <param name="httpClient"></param>
public ContosoServiceClient(HttpClient httpClient) : base (httpClient)
{

}

public ContosoServiceClient(HttpClientHandler rootHandler, DelegatingHandler[] handlers)
: base(rootHandler, handlers)
{ }

/// <summary>
/// Some task emulating getting response back
/// </summary>
/// <param name="content"></param>
/// <returns></returns>
public HttpResponseMessage DoSyncWork(string content = null)
{
return Task.Factory.StartNew(() =>
{
return DoStuff(content);
}).Unwrap().GetAwaiter().GetResult();
}


/// <summary>
/// Creates request and sends
/// </summary>
/// <param name="content">string value</param>
/// <returns></returns>
private async Task<HttpResponseMessage> DoStuff(string content = null)
{
// Construct URL
string url = "http://www.microsoft.com";

// Create HTTP transport objects
HttpRequestMessage _httpRequest = null;

_httpRequest = new HttpRequestMessage();
_httpRequest.Method = HttpMethod.Get;
_httpRequest.RequestUri = new Uri(url);

// Set content
if (content != null)
{
_httpRequest.Content = new StringContent(content);
}

// Set Headers
_httpRequest.Headers.Add("x-ms-version", "2013-11-01");

return await this.HttpClient.SendAsync(_httpRequest, new CancellationToken()).ConfigureAwait(false);
}

protected override void Dispose(bool disposing)
{
_httpClient.Dispose();
base.Dispose(disposing);
}
}

public class FabricamServiceClient : ContosoServiceClient
{
private HttpClient _httpClient;
public FabricamServiceClient(HttpClient httpClient) : base(httpClient)
{

}

public FabricamServiceClient(HttpClientHandler rootHandler, DelegatingHandler[] handlers)
: base(rootHandler, handlers)
{ }

/// <summary>
/// The idea is to have customized client override Get in the child class inheriting ServiceClient
/// And provide an instance of HttpClient.
/// This is yet another way for anyone to use their own HttpClient and override default existing client
/// </summary>
public override HttpClient HttpClient
{
get
{
if (_httpClient == null)
{
_httpClient = new HttpClient(new DelayedHandler("Delayed User Provided HttpClient after initialization"));
}

return _httpClient;
}

protected set
{
base.HttpClient = value;
}
}
}

/// <summary>
/// Custom message handler
/// </summary>
public class ContosoMessageHandler : DelegatingHandler
{
public ContosoMessageHandler() : base()
{
InnerHandler = new HttpClientHandler();
}

/// <summary>
/// Returns Contoso Rocks response
/// </summary>
/// <param name="request">HttpRequestMessage object</param>
/// <param name="cancellationToken">CancellationToken</param>
/// <returns></returns>
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
StringContent contosoContent = new StringContent("Contoso Rocks");
HttpResponseMessage response = new HttpResponseMessage(System.Net.HttpStatusCode.OK);
response.Content = contosoContent;
return await Task.Run(() => response);
}
}

/// <summary>
/// Yet another delegating handler for tests
/// </summary>
public class DelayedHandler : DelegatingHandler
{
string _handlerData;
private DelayedHandler() : base()
{
InnerHandler = new HttpClientHandler();
}

public DelayedHandler(string handlerData)
: this()
{
_handlerData = handlerData;
}

protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
StringContent contosoContent = new StringContent(_handlerData);
HttpResponseMessage response = new HttpResponseMessage(System.Net.HttpStatusCode.OK);
response.Content = contosoContent;
return await Task.Run(() => response);
}
}
}
46 changes: 40 additions & 6 deletions src/ClientRuntime/Microsoft.Rest.ClientRuntime/ServiceClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public abstract class ServiceClient<T> : IDisposable
private const string FXVERSION = "FxVersion";
private const string OSNAME = "OSName";
private const string OSVERSION = "OSVersion";
private HttpClient _httpClient;

/// <summary>
/// Indicates whether the ServiceClient has been disposed.
Expand All @@ -42,6 +43,7 @@ public abstract class ServiceClient<T> : IDisposable
/// </summary>
private string _fxVersion;

#region NET45 specific code
#if NET45
/// <summary>
/// Indicates OS Name
Expand Down Expand Up @@ -114,6 +116,7 @@ private string ReadHKLMRegistry(string path, string key)
catch { return ""; }
}
#endif
#endregion

/// <summary>
/// Gets the AssemblyInformationalVersion if available
Expand Down Expand Up @@ -196,7 +199,7 @@ private string FrameworkVersion
/// pipeline).
/// </summary>
protected HttpClientHandler HttpClientHandler { get; set; }

/// <summary>
/// Initializes a new instance of the ServiceClient class.
/// </summary>
Expand All @@ -209,6 +212,19 @@ protected ServiceClient()
{
}

/// <summary>
/// Initializes a new instance of the ServiceClient class.
/// </summary>
/// <param name="httpClient">HttpClient</param>
[System.Diagnostics.CodeAnalysis.SuppressMessage(
"Microsoft.Reliability",
"CA2000:Dispose objects before losing scope",
Justification = "The created objects should be disposed on caller's side")]
protected ServiceClient(HttpClient httpClient)
{
InitializeHttpClient(httpClient, CreateRootHandler());
}

/// <summary>
/// Initializes a new instance of the ServiceClient class.
/// </summary>
Expand Down Expand Up @@ -253,7 +269,7 @@ protected static HttpClientHandler CreateRootHandler()
/// <summary>
/// Gets the HttpClient used for making HTTP requests.
/// </summary>
public HttpClient HttpClient { get; protected set; }
public virtual HttpClient HttpClient { get; protected set; }

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@hovsepm hovsepm Mar 9, 2017

Choose a reason for hiding this comment

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

For this one - I still consider it as a potential bug hole. We use Get and Set of the HttpClient property during ServiceClient constructor code. So we put all the responsibility on the developers -> they need to be careful and do not use any objects that are not yet initialized in the overwritten property.

  1. if you walk over the call stack of ServiceClient ctr then HttpClient.Set method is called before any HttpClient.Get method call.
  2. By overriding just Get you will lose whatever was Set in the base class constructor.
  3. In your example in FabricamServiceClient class you completely ignore the HttpClient that was passed in the public FabricamServiceClient(HttpClient httpClient) constructor (because all your logic of initialization of HttpClient is happening in the HttpClient Get). Then why do you even expose a constructor that takes httpClient parameter?

Copy link
Member Author

@shahabhijeet shahabhijeet Mar 9, 2017

Choose a reason for hiding this comment

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

@hovsepm
As mentioned in the description, there are two ways to solve this problem

  1. Provide constructor overload with HttpClient parameter (more code churn, more paths to cover in tests)
  2. Make the HttpClient virtual, so classes that inherit from ServiceClient can override their own HttpClient.

#2 The sole purpose of making it virtual is to allow derived classes to override whatever was set by default, in this case a default HttpClient.
#3 The Fabricam test itself is a proof for the scenario where passed in HttpClient can still be overridden.
If we go with the virtual route, we do not necessarily have to provide an overload for the constructor for our clients.
Let's talk more about your concern about potential bug. Could you provide a potential bug that I might have missed. The entire concern on Set is called in constructor is something I would like to understand better.
I think making it HttpClient property virtual is less risky and far less code churn than providing an overload.
If we go with the "making it virtual" route, we do not need any regenerating of code for the clients via autorest for new clients.

/// <summary>
/// Gets the UserAgent collection which can be augmented with custom
Expand Down Expand Up @@ -344,6 +360,17 @@ protected virtual void Dispose(bool disposing)
"CA2000:Dispose objects before losing scope",
Justification = "We let HttpClient instance dispose")]
protected void InitializeHttpClient(HttpClientHandler httpClientHandler, params DelegatingHandler[] handlers)
{
InitializeHttpClient(null, httpClientHandler, handlers);
}

/// <summary>
/// Initialize service client with provided HttpClient
/// </summary>
/// <param name="httpClient">HttpClient</param>
/// <param name="httpClientHandler">HttpClientHandler</param>
/// <param name="handlers">List of handlers from top to bottom (outer handler is the first in the list)</param>
protected void InitializeHttpClient(HttpClient httpClient, HttpClientHandler httpClientHandler, params DelegatingHandler[] handlers)
{
HttpClientHandler = httpClientHandler;
DelegatingHandler currentHandler = new RetryDelegatingHandler();
Expand All @@ -360,16 +387,23 @@ protected void InitializeHttpClient(HttpClientHandler httpClientHandler, params
{
handler = handler.InnerHandler as DelegatingHandler;
}

handler.InnerHandler = currentHandler;
currentHandler = handlers[i];
}
}

var newClient = new HttpClient(currentHandler, false);
if (httpClient == null)
{
HttpClient = new HttpClient(currentHandler, false);
}
else
{
HttpClient = httpClient;
}

FirstMessageHandler = currentHandler;
HttpClient = newClient;
Type type = this.GetType();
SetUserAgent(type.FullName, ClientVersion);
SetUserAgent(this.GetType().FullName, ClientVersion);
}

/// <summary>
Expand Down