Implement SocketsHttpHandler.ConnectCallback #41955

geoffkizer · 2020-09-08T06:54:49Z

Fixes #41949

@scalablecory If you could take a look that would be great.

Dotnet-GitSync-Bot · 2020-09-08T06:54:52Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2020-09-08T06:54:55Z

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

geoffkizer · 2020-09-08T06:57:39Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-09-08T06:57:43Z

No pipelines are associated with this pull request.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs

geoffkizer · 2020-09-08T16:36:48Z

/azp help

azure-pipelines · 2020-09-08T16:36:53Z

Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

geoffkizer · 2020-09-08T16:37:11Z

/azp list

azure-pipelines · 2020-09-08T16:37:16Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-live-build runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono dotnet-runtime-perf runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress runtime-coreclr libraries-gcstress0x3-gcstress0xc runtime-coreclr libraries-gcstress-extra dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr clrinterpreter runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop

geoffkizer · 2020-09-08T16:37:54Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2020-09-08T16:38:07Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

geoffkizer · 2020-09-08T19:53:57Z

I pushed changes to react to API review and address feedback above. Please take a look.

geoffkizer · 2020-09-08T19:54:23Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2020-09-08T19:54:37Z

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub

Thanks.

stephentoub · 2020-09-09T00:59:33Z

src/libraries/System.Net.Http/ref/System.Net.Http.cs

+    public sealed class SocketsHttpConnectionContext
+    {
+        internal SocketsHttpConnectionContext() { }
+        public DnsEndPoint DnsEndPoint { get { throw null; } }


This may have been discussed in API review. It struck me as a little strange that we dropped the Http from HttpRequestMessage but not the Dns from DnsEndPoint: I'd have expected this to be named EndPoint for consistency. Not a big deal, though.

It wasn't discussed in API review. I think it's fine as it is -- EndPoint is actually the base class of DnsEndPoint, so calling it DnsEndPoint specifically emphasizes that it's a DnsEndPoint and not just any EndPoint.

stephentoub · 2020-09-09T01:03:38Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

+        {
+            Socket socket = new Socket(AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp);
+            socket.NoDelay = true;
+            socket.DualMode = true;


What happens if the OS doesn't support IPv6? I'm wondering if this should instead use the 2-arg ctor:

var socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };

This is just transferring over the existing code from SocketsConnectionFactory. But I agree, it seems a bit questionable.

@scalablecory Do you know why SocketsConnectionFactory was doing this?

I'm going to change this as @stephentoub suggested. @scalablecory if you have any thoughts here, let me know.

was likely a bug.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

stephentoub · 2020-09-09T01:09:00Z

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

+                        Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
+                        s.Bind(new IPEndPoint(IPAddress.Loopback, 0));
+                        await s.ConnectAsync(context.DnsEndPoint, token);
+                        s.NoDelay = true;


One of the other commonly discussed use cases is tweaking keep alive settings on the socket. Just for completeness, might be worth adding a test around that, too, even though it's unlikely to flag something this test doesn't.

...aries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpConnectionContext.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

geoffkizer · 2020-09-09T08:57:14Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2020-09-09T08:57:26Z

Azure Pipelines successfully started running 1 pipeline(s).

scalablecory

lgtm

scalablecory · 2020-09-09T20:45:44Z

...aries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpConnectionContext.cs

+        private readonly DnsEndPoint _dnsEndPoint;
+        private readonly HttpRequestMessage _requestMessage;


nit: these could be auto props.

geoffkizer · 2020-09-10T16:14:31Z

/backport to release/5.0-rc2

github-actions · 2020-09-10T16:16:05Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/248358609

Implement SocketsHttpHandler.ConnectCallback

1623afc

Dotnet-GitSync-Bot added area-System.Net.Http new-api-needs-documentation labels Sep 8, 2020

update xml doc for ConnectCallback property

8983c36

geoffkizer added this to the 6.0.0 milestone Sep 8, 2020

stephentoub reviewed Sep 8, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

stephentoub reviewed Sep 8, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated Show resolved Hide resolved

danmoseley reviewed Sep 8, 2020

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs Outdated Show resolved Hide resolved

wfurt reviewed Sep 8, 2020

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs Outdated Show resolved Hide resolved

use Socket.OSSupportsUnixDomainSockets in tests

f37ae90

ManickaP reviewed Sep 8, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

Geoffrey Kizer added 6 commits September 8, 2020 11:42

add ConnectionContext

7152086

use NSE instead of IOE for sync path

5910266

use ConnectionContext

97e50d2

add test for context validation

f9b8585

fix browser build, and HttpRequestMessage -> RequestMessage

f8ac649

remove SocketsConnectionFactory

27b8b86

stephentoub reviewed Sep 9, 2020

View reviewed changes

address feedback

f2ec912

scalablecory approved these changes Sep 9, 2020

View reviewed changes

geoffkizer merged commit 8b8c390 into dotnet:master Sep 10, 2020

github-actions bot mentioned this pull request Sep 10, 2020

[release/5.0-rc2] Implement SocketsHttpHandler.ConnectCallback #42075

Merged

martincostello mentioned this pull request Sep 15, 2020

[.NET 5 RC1] "HttpRequestException: Address family not supported by protocol (127.0.0.1:9001)" thrown when Lambda invoked aws/aws-lambda-dotnet#730

Closed

aik-jahoda mentioned this pull request Sep 16, 2020

Documentation for System.Net.Http.SocketsHttpHandler.ConnectCallback dotnet/dotnet-api-docs#4861

Merged

geoffkizer deleted the connectioncallback branch November 7, 2020 23:39

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SocketsHttpHandler.ConnectCallback #41955

Implement SocketsHttpHandler.ConnectCallback #41955

geoffkizer commented Sep 8, 2020

Dotnet-GitSync-Bot commented Sep 8, 2020

ghost commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

stephentoub left a comment

stephentoub Sep 9, 2020

geoffkizer Sep 9, 2020

stephentoub Sep 9, 2020

geoffkizer Sep 9, 2020

geoffkizer Sep 9, 2020

scalablecory Sep 9, 2020

stephentoub Sep 9, 2020

geoffkizer commented Sep 9, 2020

azure-pipelines bot commented Sep 9, 2020

scalablecory left a comment

scalablecory Sep 9, 2020

geoffkizer commented Sep 10, 2020

github-actions bot commented Sep 10, 2020

		private readonly DnsEndPoint _dnsEndPoint;
		private readonly HttpRequestMessage _requestMessage;

Implement SocketsHttpHandler.ConnectCallback #41955

Implement SocketsHttpHandler.ConnectCallback #41955

Conversation

geoffkizer commented Sep 8, 2020

Dotnet-GitSync-Bot commented Sep 8, 2020

ghost commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

geoffkizer commented Sep 8, 2020

azure-pipelines bot commented Sep 8, 2020

stephentoub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Sep 9, 2020

azure-pipelines bot commented Sep 9, 2020

scalablecory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Sep 10, 2020

github-actions bot commented Sep 10, 2020