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

Validate and cover HttpClient extensibility #40624

Open
1 of 8 tasks
antonfirsov opened this issue Aug 10, 2020 · 11 comments
Open
1 of 8 tasks

Validate and cover HttpClient extensibility #40624

antonfirsov opened this issue Aug 10, 2020 · 11 comments
Labels
area-System.Net.Http test-enhancement Improvements of test source code
Milestone

Comments

@antonfirsov
Copy link
Member

A follow-up on #40506 (comment).

In #1793 we identified 8 scenarios, which could be solved by using a custom ConnectionFactory in SocketsHttpHandler.ConnectionFactory. Ideally the design enables them, however most of those scenarios are still not validated. We need end-to-end tests in System.Net.Http to validate and cover these scenarios:

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Aug 10, 2020
@ghost
Copy link

ghost commented Aug 10, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@antonfirsov
Copy link
Member Author

antonfirsov commented Aug 10, 2020

@scalablecory @geoffkizer @stephentoub questions:

@scalablecory
Copy link
Contributor

Correct

@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Aug 11, 2020
@karelz karelz added this to the 6.0.0 milestone Aug 11, 2020
@karelz karelz added the test-enhancement Improvements of test source code label Aug 11, 2020
@geoffkizer
Copy link
Contributor

geoffkizer commented Aug 11, 2020

I think all of these scenarios boil down to ensuring that the hooks we provide are actually working as intended.

Specifically we should add some tests that
(1) Override CreateSocket and specify custom socket behavior (e.g. TCP KeepAlive or whatever), and ensure that the resulting connection has the expected behavior
(2) Given a DnsEndpoint, override the connect logic with your own that does custom DNS resolution (which can be trivial, i.e. always connect to localhost or whatever)
(3) Ensure we can intercept all reads/writes on the raw connection and that (a) the values we get are as expected; (b) we can modify the values on the way in/out

@stephentoub
Copy link
Member

I think all of these scenarios boil down to ensuring that the hooks we provide are actually working as intended.

I agree we should do such a general approach for good measure. But we also need to validate the specific scenarios we were targeting. Case in point, the UDS scenario that failed because the implementation was trying to disable nagle and that throws with a Unix Domain Socket wouldn't have been caught by the above plan.

@geoffkizer
Copy link
Contributor

need to validate the specific scenarios we were targeting

I agree; I think the question is how to do this. Does this mean unit tests? Sample code? What? I don't think we want/need to actually implement bandwidth monitoring in our unit tests, for example.

Case in point, the UDS scenario that failed

I agree, we should have a unit test for this.

@antonfirsov
Copy link
Member Author

Re UDS:
we have a unit test for the specific subfeature of succesfully creating and using UDS Connection with SocketConnectionFactory. As far as I understand, @stephentoub means is to have actual end-to-end functional tests utilizing HttpClient with a UDS test Http server to make sure there are no other things broken.

@mjsabby
Copy link
Contributor

mjsabby commented Aug 20, 2020

One important use case we'll be using this new abstraction for, is improving diagnostics when calling a remote peer that is actually a Load Balancer (e.g. Azure Load Balancer).

What we're planning to do is call the remote service, remote.foo.com, connect to it, request a well-known url say /information, which gives us some diagnostics information, maybe a real IPv4/IPv6 or in a microservices world "InstanceId" and we attach to that to this connection object.

Only after this piece of work is done we make this connection available for use. This is super useful, because when 100 HTTP requests later for whatever reason the http request fails, we can tell you what instance of the service we associated in that first connection.

This information can then be provided to the service we were calling and they can debug further.

@geoffkizer
Copy link
Contributor

That seems like a good thing for us to write a sample for, and validate that this works. @antonfirsov is this something you plan to tackle?

@ManickaP ManickaP self-assigned this Aug 28, 2020
@davidfowl
Copy link
Member

I wrote a sample of an in memory transport and plugged it into HttpClient:

https://github.com/davidfowl/CommunityStandUpNet5/blob/89b849a96cf86904d52ca70adb9d4b37b0baf1bd/APIPlayground/Program.cs#L71-L118

@davidfowl
Copy link
Member

@JamesNK has been working on a named pipes transport for gRPC.

@ManickaP ManickaP removed their assignment Aug 31, 2020
@karelz karelz modified the milestones: 6.0.0, Future May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Http test-enhancement Improvements of test source code
Projects
None yet
Development

No branches or pull requests

9 participants