feat(iot-service, shared): Add support for configuring http connection lease timeouts #1874
Conversation
timtay-microsoft
changed the title
timtay-microsoft
requested review from
abhipsaMisra,
azabbasi,
barustum,
bikamani,
drwill-ms,
jamdavi and
vinagesh
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
timtay-microsoft
changed the title
…timeout in Http service clients Also fixes #1865 by making the default connection lease timeout no longer infinite
timtay-microsoft
force-pushed
the
timtay/takeTwo
branch
from
4c86e8b to
02e7677
Compare
| HttpMessageHandler httpMessageHandler = new HttpClientHandler(); | ||
| #endif | ||
| // Currently, the connection lease timeout isn't configurable in this client. This may be worth adding later though | ||
| ServicePointHelpers.SetLimits(httpMessageHandler, uri, ServicePointHelpers.DefaultConnectionLeaseTimeout); |
There was a problem hiding this comment.
Since there isn't a great way to add the option to configure this connection lease timeout, I'll just leave it as the default for now. We can always add a constructor later that takes this as a configurable option if a customer asks for it
| servicePoint.ConnectionLeaseTimeout = connectionLeaseTimeoutMilliseconds; | ||
| break; | ||
| #if NETCOREAPP | ||
| case SocketsHttpHandler socketsHttpHandler: |
There was a problem hiding this comment.
netcoreapp1.0-2.0 won't send the messageHandler as a socket http handler, so this switch block won't be executed, but do you think excluding those versions in the if-def is in any way useful?
There was a problem hiding this comment.
I'll make this if-def match the if-def I have where we decide to make this handler a SocketsHttpHandler vs HttpClientHandler to make this moot
| DelegatingHandler handler = handlers[i]; | ||
| // Non-delegating handlers are ignored since we always | ||
| // have RetryDelegatingHandler as the outer-most handler | ||
| while (handler.InnerHandler is DelegatingHandler) |
There was a problem hiding this comment.
curious if you could have done: while (handler.InnerHandler is DelegatingHandler delegatingHandler)
There was a problem hiding this comment.
Possibly. I didn't want to alter this code more than I needed to though since it was copied from track 2 and is designed to mimic the behavior that we already had. I just wanted this method to accept the more generic HttpMessageHandler rather than a HttpClientHandler
| // have RetryDelegatingHandler as the outer-most handler | ||
| while (handler.InnerHandler is DelegatingHandler) | ||
| { | ||
| handler = handler.InnerHandler as DelegatingHandler; |
There was a problem hiding this comment.
Should only use as if you are unsure it is that type, because it will otherwise return null. When you are sure (and you are because of the check in the while loop), you should just cast (assertive).
There was a problem hiding this comment.
Good point. This code was copied from the track 2 folks, so I missed this
| @@ -27,5 +27,19 @@ public HttpTransportSettings() | |||
| /// especially when MQTT and AMQP ports are disallowed to the internet. | |||
| /// </remarks> | |||
| public IWebProxy Proxy { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// How long, in milliseconds, a given cached TCP connection created by this client's HTTP layer will live before being closed. | |||
…n lease timeouts (Azure#1874) Also fixes Azure#1865 by making the default connection lease timeout no longer infinite
(This PR works now) The gist of it is that the servicepointmanager timeout properties work fine for .NET framework applications, but .NET core requires you to construct a SocketsHttpHandler as the basehandler in order to solve this issue. This very closely mimics how Azure.Core solves this same issue.