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

Add ConnectCallback to SocketsHttpHandler #41806

Closed
wants to merge 1 commit into from
Closed

Add ConnectCallback to SocketsHttpHandler #41806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Common/src/System/Net/Http/HttpHandlerDefaults.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.IO;
using System.Threading;
using System.Threading.Tasks;

namespace System.Net.Http
{
Expand Down
1 change: 1 addition & 0 deletions src/System.Net.Http/ref/System.Net.Http.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ public SocketsHttpHandler() { }
public System.Net.IWebProxy Proxy { get { throw null; } set { } }
public System.TimeSpan ResponseDrainTimeout { get { throw null; } set { } }
public System.Net.Security.SslClientAuthenticationOptions SslOptions { get { throw null; } set { } }
public System.Func<string, int, System.Threading.CancellationToken, System.Threading.Tasks.ValueTask<System.IO.Stream>> ConnectCallback { get { throw null; } set { } }
public bool UseCookies { get { throw null; } set { } }
public bool UseProxy { get { throw null; } set { } }
protected override void Dispose(bool disposing) { }
Expand Down
4 changes: 2 additions & 2 deletions src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,11 +622,11 @@ public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool doRe
case HttpConnectionKind.Http:
case HttpConnectionKind.Https:
case HttpConnectionKind.ProxyConnect:
stream = await ConnectHelper.ConnectAsync(_host, _port, cancellationToken).ConfigureAwait(false);
stream = await Settings._customConnect(_host, _port, cancellationToken).ConfigureAwait(false);
break;

case HttpConnectionKind.Proxy:
stream = await ConnectHelper.ConnectAsync(_proxyUri.IdnHost, _proxyUri.Port, cancellationToken).ConfigureAwait(false);
stream = await Settings._customConnect(_proxyUri.IdnHost, _proxyUri.Port, cancellationToken).ConfigureAwait(false);
break;

case HttpConnectionKind.ProxyTunnel:
Expand Down
7 changes: 7 additions & 0 deletions src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Net.Security;
using System.Threading;
using System.Threading.Tasks;

namespace System.Net.Http
{
Expand Down Expand Up @@ -48,6 +52,8 @@ internal sealed class HttpConnectionSettings

internal IDictionary<string, object> _properties;

internal Func<string, int, CancellationToken, ValueTask<Stream>> _customConnect = ConnectHelper.ConnectAsync;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it could be argued that the string and int parameters of the delegate do not clearly communicate meaning. Any reason why we can't use a custom delegate here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can and I agree, but @stephentoub was against it and I didn't think it is a huge enough benefit to fight for it.

I think that adding a second host/port pair for proxy would be really confusing though, so we will need either a new delegate type or a new EventArgs-like type which we'd need to allocate.

Copy link
Member

@stephentoub stephentoub Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to have something like:

Func<HttpRequestMessage, CancellationToken, ValueTask<Stream>>

and I don't think a custom delegate adds anything meaningful there. If we end up just taking string/int (for whatever reason... from reading on in the PR it seems like that might be necessary), or worse multiple string/int pairs, then a custom delegate is fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option I've been thinking of tonight is to give HttpConnectionPool an informational base class so that we can pass it directly to the callback. No allocations that way, and it gives us an option to expose more in the future if we need to.

I don't think HttpRequestMessage makes sense when multiple requests can be sent on a single connection. Granted this is an API for advanced usage so I think we do have some leeway here, but it'd be very easy to assume the wrong behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to allocate a new delegate for every HttpConnectionSettings. While that's unlikely to be particularly meaningful, it's also unnecessary given that ConnectHelper.ConnectAsync is static.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactoring oversight; thanks.


public HttpConnectionSettings()
{
bool allowHttp2 = AllowHttp2;
Expand Down Expand Up @@ -88,6 +94,7 @@ public HttpConnectionSettings CloneAndNormalize()
_useCookies = _useCookies,
_useProxy = _useProxy,
_allowUnencryptedHttp2 = _allowUnencryptedHttp2,
_customConnect = _customConnect
};
}

Expand Down
11 changes: 11 additions & 0 deletions src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.IO;
using System.Net.Security;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -32,6 +33,16 @@ private void CheckDisposedOrStarted()
}
}

public Func<string, int, CancellationToken, ValueTask<Stream>> ConnectCallback
{
get => _settings._customConnect;
set
{
CheckDisposedOrStarted();
_settings._customConnect = value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to allow null to be set here? Won't we null ref later when we try to invoke this?

}
}

public bool UseCookies
{
get => _settings._useCookies;
Expand Down
36 changes: 36 additions & 0 deletions src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2689,5 +2689,41 @@ public async Task GetAsync_InvalidUrl_ExpectedExceptionThrown()
await Assert.ThrowsAsync<HttpRequestException>(() => client.GetStringAsync(invalidUri));
}
}

[Fact]
public async Task ConnectCallback_Success()
{
if (!UseSocketsHttpHandler || UseHttp2) return;

await LoopbackServer.CreateClientAndServerAsync(
async uri =>
{
bool dialerCalled = false;

Func<string, int, CancellationToken, ValueTask<Stream>> dialer = (host, port, token) =>
{
dialerCalled = true;

byte[] buffer = Encoding.ASCII.GetBytes("HTTP/1.1 200 OK\r\nConnection: close\r\nContent-Length: 3\r\nContent-Type: text/plain\r\n\r\nfoo");

var stream = new MemoryStream(buffer);
var delegateStream = new DelegateStream(canReadFunc: () => true, canWriteFunc: () => true, readFunc: stream.Read, writeFunc: delegate { });
return new ValueTask<Stream>(delegateStream);
};

using var handler = new SocketsHttpHandler();
handler.ConnectCallback = dialer;

using HttpClient client = CreateHttpClient(handler);

// If GetStringAsync fails with a connect timeout, the dialer is not getting called -- it's hitting a loopback socket that will never accept.
string result = await client.GetStringAsync(uri);

Assert.Equal("foo", result);
Assert.True(dialerCalled);
},
server => Task.CompletedTask,
new LoopbackServer.Options { ListenBacklog = 0 });
}
}
}