dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.38k stars 4.75k forks source link

Validate and cover HttpClient extensibility #40624

Open antonfirsov opened 4 years ago

antonfirsov commented 4 years ago

A follow-up on https://github.com/dotnet/runtime/pull/40506#discussion_r466948246.

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:

ghost commented 4 years ago

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

antonfirsov commented 4 years ago

@scalablecory @geoffkizer @stephentoub questions:

scalablecory commented 4 years ago

Correct

geoffkizer commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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?

davidfowl commented 4 years ago

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 commented 4 years ago

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