Feature/optimization http client instance#57
Feature/optimization http client instance#57TomPallister merged 11 commits intoThreeMammals:developfrom geffzhang:feature/Optimization-HttpClient-instance
Conversation
mergecode
merge rate limit
merge newest code
merge newest code
Update Ocelot configuration without downtime
fix：updated readme to explain how to run integration tests
|
@geffzhang ignore my previous comment. Unfortunately you cannot set HttpClientHandler after HttpClient has been instantiated. |
TomPallister
left a comment
TomPallister
left a comment
There was a problem hiding this comment.
I have left some comments. Please review.
I have also noticed we do not need cookie container as cookies are just set as headers...I did not realise this. If we did use the cookie container im not sure implementation of caching HttpClient would be thread safe. However cookies are stored on HttpRequestMessage which is instantiated per request and is thread safe.
| _memoryCache = memoryCache; | ||
| } | ||
|
|
||
| public void Set(string id, IHttpClient counter, TimeSpan expirationTime) |
There was a problem hiding this comment.
string id maybe call this key, IHttpClient counter maybe call this httpClient
|
|
||
| namespace Ocelot.Requester | ||
| { | ||
| public interface IHttpClientMessageCacheHandler |
There was a problem hiding this comment.
Maybe call this IHttpClientCache
|
|
||
| namespace Ocelot.Requester | ||
| { | ||
| public class MemoryHttpClientMessageCacheHandler : IHttpClientMessageCacheHandler |
There was a problem hiding this comment.
I would call this MemoryCacheHttpClientCache
| { | ||
| public class HttpClientHttpRequester : IHttpRequester | ||
| { | ||
| private IHttpClientMessageCacheHandler _cacheHandlers; |
| baseUrl = $"{baseUrl}{request.QosProvider.CircuitBreaker.CircuitBreakerPolicy.PolicyKey}"; | ||
| } | ||
|
|
||
| IHttpClient httpClient = _cacheHandlers.Get(baseUrl); |
There was a problem hiding this comment.
IHttpClient to be var (we use var everywhere else)
| internal class HttpClientBuilder : IHttpClientBuilder | ||
| { | ||
| private readonly Dictionary<int, Func<DelegatingHandler>> _handlers = new Dictionary<int, Func<DelegatingHandler>>(); | ||
| private TimeSpan? _timeout; |
|
|
||
| var client = new HttpClient(CreateHttpMessageHandler(httpclientHandler)); | ||
|
|
||
| if (_timeout.HasValue) |
There was a problem hiding this comment.
never set so we dont need this code
| client.Timeout = _timeout.Value; | ||
| } | ||
|
|
||
| if (_defaultHeaders == null) |
There was a problem hiding this comment.
never set so we dont need this code
| return new HttpClientWrapper(client); | ||
| } | ||
|
|
||
| foreach (var header in _defaultHeaders) |
There was a problem hiding this comment.
never set so we dont need this code
| using (var handler = new HttpClientHandler { CookieContainer = request.CookieContainer }) | ||
| builder.WithCookieContainer(request.CookieContainer); | ||
|
|
||
| string baseUrl = $"{request.HttpRequestMessage.RequestUri.Scheme}://{request.HttpRequestMessage.RequestUri.Authority}"; |
There was a problem hiding this comment.
Maybe make a private method to do this..will look nicer.
geffzhang
left a comment
geffzhang
left a comment
There was a problem hiding this comment.
I has modified according to your views
| return this; | ||
| } | ||
|
|
||
| public IHttpClientBuilder WithDefaultRequestHeaders(Dictionary<string, string> headers) |
2 participants
Instantiating an HttpClient for every request can possible exhaust the number of sockets available under heavy loads. Microsoft patterns & practices https://github.com/mspnp/performance-optimization/blob/master/ImproperInstantiation/docs/ImproperInstantiation.md goes into detail on this issue, How to Cache HttpClient Instances? I now use MemoryCache, How to do best