Add ConnectCallback to SocketsHttpHandler

[scalablecory]

Adds SocketsHandler.ConnectCallback. Resolves #35404

[eiriktsarpalis]

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?

[scalablecory]

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.

[stephentoub]

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.

[scalablecory]

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.

[stephentoub]

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.

[scalablecory]

Refactoring oversight; thanks.

[stephentoub]

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

[stephentoub]

@scalablecory, you marked this PR as "no review". Why put up a PR if it's not supposed to be reviewed?

[davidfowl]

Some questions:

• Would this allow somebody to implement their own SSLStream (or get inbetween the SSL stream)? It doesn't look like it. One of the scenarios is to allow logging data after the SSlStream is applied.
• Would this allow decorating the existing transport? There are scenarios where people want to plug in to whatever transport is already there without having to know what was originally there.
• There's a scenario where the caller wants to take control of connection pooling. This might be the wrong extensibility point though...

I think the callback needs to look like this:

public class SocketsHttpHandler
{
    Func<Stream, HttpRequestMessage, CancellationToken, ValueTask<Stream>> ConnectCallback { get; set; }
}

OR maybe we need to callbacks, one to get the transport stream and then one applied after that Stream is established:

public class SocketsHttpHandler
{
    Func<HttpRequestMessage, CancellationToken, ValueTask<Stream>> ConnectCallback { get; set; }
    Func<Stream, ValueTask<Stream>> MiddlewareCallback { get; set; }
}

I don't have a good name yet but you get the idea 😄

The separation might be cleaner as it allows code to plug wrapping streams into the pipeline (like a delegating handler) without having to know about what transport is in use.

[scalablecory]

@scalablecory, you marked this PR as "no review". Why put up a PR if it's not supposed to be reviewed?

People can certainly review it if they want to (it's definitely a small change 😀), but I'm planning to iterate this a little bit so I don't feel that it's an appropriate use of time yet to ask the whole party in for a look. The purpose at this time is to prove out that such a thing could be used cleanly to inform an API choice.

Would this allow somebody to implement their own SSLStream (or get inbetween the SSL stream)? It doesn't look like it. One of the scenarios is to allow logging data after the SSlStream is applied.

Yea, it won't work for that. I haven't seen this scenario asked for before. Do you have a need for this?

Would this allow decorating the existing transport? There are scenarios where people want to plug in to whatever transport is already there without having to know what was originally there.

Yes, the idea is that a user could save a copy of the default ConnectCallback if they just want to put a shim around the base implementation.

There's a scenario where the caller wants to take control of connection pooling. This might be the wrong extensibility point though...

That seems like a much larger change. I'd want to see some samples of what custom pooling people would implement before tackling that -- it seems like a very rare need and we may be able to handle many asks just by making the current pool more configurable.

[davidfowl]

Yea, it won't work for that. I haven't seen this scenario asked for before. Do you have a need for this?

Ideally this would be a pipeline. Transport -> Custom Before - SSL -> Custom After -> Http stack. Internals teams have asked for this control before.

Yes, the idea is that a user could save a copy of the default ConnectCallback if they just want to put a shim around the base implementation.

Yes that works.

That seems like a much larger change. I'd want to see some samples of what custom pooling people would implement before tackling that -- it seems like a very rare need and we may be able to handle many asks just by making the current pool more configurable.

Maybe, but lets not pollute this PR. It seems like at least one of the above scenarios works.

[stephentoub]

The purpose at this time is to prove out that such a thing could be used cleanly to inform an API choice.

Ok. That doesn't require a PR. You can just share a link to your branch / commit in the issue about the API proposal.

[stephentoub]

@scalablecory, since the API design for this hasn't been approved, and since folks can continue to comment on your commits that serve as an example implementation of one design, I'm going to close this no-review/no-merge PR. Thanks.