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

[Proposal] HttpConnection Unification and Optimisation #31

Open
stevejgordon opened this issue Aug 20, 2021 · 0 comments
Open

[Proposal] HttpConnection Unification and Optimisation #31

stevejgordon opened this issue Aug 20, 2021 · 0 comments
Labels
enhancement New feature or request performance

Comments

@stevejgordon
Copy link
Collaborator

Current situation

Today, we have two implementations for IConnection. HttpConnection is used for .NET Core while for .NET Fx, we use HttpWebRequestConnection. This requirement maintenance of two similar, but distinct code paths.

We also have an implementation of HttpClientFactory used internally to pool the underlying HttpMessageHandlers. This is designed to re-use sockets where possible, while also ensuring stale handlers are removed such that connections refresh periodically.

Proposals

HttpClient Unification

Review whether we can safely unify these to use HttpClient in the case of all target frameworks. This simplifies the design and testing of future features implemented at the lowest level of the transport.

Removal of HttpClientFactory behaviours

There is overhead involved in maintaining the HttpMessageHandler pool and the machinery required for the pool to function. This includes allocations and additional dispatch time. Since .NET Core introduced the SocketsHttpHandler, it includes built-in pooling support with configuration options to control the lifetime of sockets in that pool. We could leverage this, rather than maintaining our own pool. On .NET Fx, we could use ServicePointManager to achieve the same goal (once unified on HttpClient APIs).

Things to consider

While this proposal seems simple in principle, there is a gotcha. SocketsHttpHandler until .NET 6.0 does not utilise the internal DiagnosticsHandler which emits diagnostic events and ensure trace headers are forwarded. This presents a problem since we need to use the SocketsHttpHandler in order to configure the pooling timeouts, but in doing so, we lose the diagnostics capabilities. This PR for .NET 6.0 partially addresses the problem for the trace parent header propagation at least but perhaps not all diagnostic capabilities.

We need to test what is/isn't supported and decide if we have conditional code to remove HttpMessageHandler requirements only for .NET 6.0. Alternatively, we may be able to roll our own version of the DiagnosticsHandler in such a way as to allow the removal of HttpMessageHandler for all target frameworks.

While it would diverge the code-base to handle .NET 6.0 differently, if the diagnostics support is sufficient there when using SocketsHttpHandler, we could offer an initial benefit to consumers on this runtime.

Next steps

  • Remove HttpWebRequestConnection usage for .NET Fx targets and test in .NET 4.61 upwards.
  • Investigate the impact of the loss of DiagnosticHandler capabilities when using SocketsHttpHandler on .NET Core 3.1, .NET 5.0 and .NET 6.0. An outcome of this investigation will drive subsequent planning of the best design.
@stevejgordon stevejgordon changed the title [WIP] HttpConnection Unification and Optimisation [Proposal] HttpConnection Unification and Optimisation Aug 20, 2021
@stevejgordon stevejgordon added enhancement New feature or request performance labels Aug 20, 2021
@stevejgordon stevejgordon added the GA Target completion before GA 8.0.0 label Jan 25, 2022
@stevejgordon stevejgordon removed the GA Target completion before GA 8.0.0 label Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance
Projects
None yet
Development

No branches or pull requests

1 participant