SocketAsyncEventArgs.ConnectSocket can't be explicitly cleared

[stephentoub]

This means SocketAsyncEventArgs that are pooled (as they're meant to be) and are used to establish connections can end up keeping a Socket instance alive indefinitely (until the SocketAsyncEventArgs is used for another connect operation).

This affects SocketsHttpHandler, for example. It pools SAEAs used to establish connections. If an SAEA ends up back in the pool, and new connections aren't established frequently enough to cause it to be reused, and a developers drops an HTTP/1.1 response stream before reaching the end of the response body and without disposing of it, the connection may remain open (until the server closes it or the pool reuses the SAEA for another connection).

I'm not sure how to address this without new API, e.g. a setter on ConnectSocket that can be set to null, a Clear method on SAEA that clears all such state, etc.

cc: @davidsh, @geoffkizer

[geoffkizer]

Can we just not pool them in SocketsHttpHandler? I'm not sure it helps that much to pool, does it?

Would be nice to fix this longer-term, but fixing the SocketsHttpHandler issue seems like the urgent issue.

[stephentoub]

Would be nice to fix this longer-term, but fixing the SocketsHttpHandler issue seems like the urgent issue.

I'm not sure how urgent it is. It only affects cases where the socket isn't explicitly disposed (e.g. someone doesn't read to the end of a response body and also doesn't dispose of the response/stream) and where so few connections are being made that the SAEA isn't reused within a reasonable amount of time. In such a case it means we keep the connection open until either the SAEA gets reused and/or the server closes the connection.

I'm not sure it helps that much to pool, does it?

It helps non-trivially, not only with perf, but also with the ability to cancel setting up connections. I expect it would be destabilizing to try to change that now.

[geoffkizer]

It helps non-trivially, not only with perf, but also with the ability to cancel setting up connections. I expect it would be destabilizing to try to change that now.

Can't we just remove the pooling and still use SAEA and get the cancellation from it? The only downside is allocating a new SAEA each time, instead of reusing them.

[stephentoub]

The only downside is allocating a new SAEA each time, instead of reusing them.

Yes, but that's a big and measurable downside.

This is not a new issue. And no one to my knowledge has noticed. If you're dropping a response in a way that leads to this, then either you're going to make another request resulting in a new connection and it very likely won't matter, or you're not going to make another request, in which case it likely won't matter, either.

[stephentoub]

If we believe it must be addressed in 3.0, my perspective is the answer is to do it right and add the necessary API. It could be as simple as adding a setter that only allows null. The whole purpose of SAEA is to cache and reuse it; if we don't feel comfortable using it for that, that's a problem.

[davidsh]

This affects SocketsHttpHandler, for example. It pools SAEAs used to establish connections. If an SAEA ends up back in the pool, and new connections aren't established frequently enough to cause it to be reused, and a developers drops an HTTP/1.1 response stream before reaching the end of the response body and without disposing of it, the connection may remain open (until the server closes it or the pool reuses the SAEA for another connection).
I'm not sure how to address this without new API, e.g. a setter on ConnectSocket that can be set to null, a Clear method on SAEA that clears all such state, etc.

For 3.0, can't we use reflection to fix the critical scenario here? And then post 3.0, add a real API.

[davidsh]

Also, in the old days, all of this networking code was in a single DLL. So, we could simply add 'internal' methods instead of public APIs. We only need a public API here because we have separate System.Net.Sockets.dll vs. System.Net.Http.dll for example.

[stephentoub]

can't we use reflection to fix the critical scenario here?

That's what I have locally as part of validating the issue. I don't love it, but yeah, we could do it if necessary.

We only need a public API here because we have separate System.Net.Sockets.dll vs. System.Net.Http.dll for example.

And for everyone else using SAEA.

[stephentoub]

So... it ends up being worse than I initially thought. Not only is the SAEA storing the current socket and not clearing it, but it's storing a helper "multiple connect" object, which stores the socket as well and doesn't clear it, and it's actually creating a second new SocketAsyncEventArgs instance on each call as an internal helper, which in turn is also storing and not clearing the socket. Ugh. This all needs to be cleaned up, but it's too big and fundamental a change to make now for 3.0. I'll just bite my tongue and remove the SAEA caching for now.

[geoffkizer]

For the future, it would be nice to enable the same functionality on Socket without SAEA, i.e. cancellable connect with reasonable perf/allocation. I'd love to get to a place where SAEA is never actually needed. Maybe there's a reason this is not achievable, but it would be good to understand that better.

[karelz]

Triage:
We need to add ClearSocket property (or maybe the whole structure) -- this will require new API.

We need overall refactoring of ConnectAsync methods

Would be nice in 5.0, but can be pushed further if necessary.