Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

SocketsHttpHandler, support for custom endpoint address resolving #27937

Closed
wants to merge 1 commit into from
Closed

SocketsHttpHandler, support for custom endpoint address resolving #27937

wants to merge 1 commit into from

Conversation

AppBeat
Copy link

@AppBeat AppBeat commented Mar 10, 2018

Hello.

I created my first commit to corefx fork / System.Net.Http project with small improvement to SocketsHttpHandler where user can now have more control over endpoint address resolving.

For example user can prefer IPv6 resolving over IPv4 version or even write custom logic which completely overrides system DNS resolve.

This feature is optional and should be backwards compatible.

Please check if this feature could be generally useful :)

@stephentoub @geoffkizer @karelz

@dnfclas
Copy link

dnfclas commented Mar 10, 2018

CLA assistant check
All CLA requirements met.

@stephentoub
Copy link
Member

Thanks for your interest in contributing, @AppBeat!

We have an API process that we use for adding new public surface area, which this does:
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md
We like to start with an issue that discusses the problem being addressed and includes a proposal for addressing it, which folks can then discuss and hash out the design on, before we then decide whether to add such APIs. As such, I'm going to close this PR for now, but I'd encourage you to open an issue with your ideas, and obviously you can link to your commit(s) that provides an example implementation to help flesh it out.

A few initial thoughts:

  • I generally like the idea of allowing for this customization. We've talked about doing it before, as a way to allow providing more control over DNS resolution in particular.
  • I don't believe HttpRequestMessage should be modified for this. It can purely be a feature of the particular handler.
  • We would need to do it in a way that properly factored in connection pooling; the way it's implemented in your commit, the same pool could actually end up including connections to different end points, which could be problematic.
  • There's no need for a custom delegate here; we generally prefer to use Func<...> and Action<...> rather than introducing new delegate types.
  • I expect we'd want to have the delegate return a ValueTask<EndPoint> rather than Task<EndPoint>, as it's likely the resolver would be able to return a value synchronously (e.g. if it maintained its own DNS cache).

Thanks, again.

@AppBeat
Copy link
Author

AppBeat commented Mar 10, 2018

Thanks! I will try to improve this based on your feedback.

@AppBeat
Copy link
Author

AppBeat commented Mar 11, 2018

Hello.

I checked-in first prototype on https://github.com/AppBeat/corefx fork.

All changes are now done on SocketsHttpHandler folder and should be 100% compatible with old code.

As Mr. Toub noted, I had most issues with connection pool and had to change my resolving strategy there.
If custom address resolving is used I now try to resolve address inside GetConnectionKey method (or more precisely GetConnectionKeyAsync). If hostname www.some-domain.com is originally used then ResolvedIp:Port is used as ConnectionKey. Original HttpRequestMessage is not changed in any way.

Main goals:
-feature is optional and should not change current behaviour in any way
-if feature is not used it should not affect existing performance

TODOs:
-possible naming refactorings
-SocketsHttpHandler.ResolveEndpointAsync -> should it be static or not
-two new exceptions should be "translated" (and check if correct exception type is used)
-testing: since I am new here, I couldn't figure out how easily could I test this new feature, since new properties are not visible in VS System.Net.Http.Functional.Tests :(

Background:
At AppBeat website monitoring we use .NET Core implementation of non-HttpClient for such functionality and it works very well in production. However, if similar functionality would be available in SocketsHttpHandler we would rather use this. Since this functionality would offer more flexibility and control I think it may be generally useful. However I may be biased here :)

Possible use cases:
-one could implement ResolveEndpointAsync to use Google DNS or other DNS server for resolving addresses, without changing system settings
-one could implement ResolveEndpointAsync to use round-robin for resolving IP Addresses
-better control over IPv4/IPv6 resolving
-could be used in functional testing to resolve test IP addresses

Kind regards,
Vladimir

@karelz @rmkerr @davidsh @Caesar1995 @wfurt

@stephentoub
Copy link
Member

@AppBeat, following https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md, could you open an issue (rather than PR) with the proposed API surface area and this information? Thanks!

@AppBeat
Copy link
Author

AppBeat commented Mar 11, 2018

New feature submitted at https://github.com/dotnet/corefx/issues/27949

Kind regards,
Vladimir

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants