SocketsHttpHandler: Ensure that we don't initiate connection creation under the pool lock #32524
Conversation
|
@dotnet-bot test outerloop linux x64 debug build |
| } | ||
| catch | ||
| { | ||
| DecrementConnectionCount(); |
There was a problem hiding this comment.
I noticed this in the preexisting code as well, but I'm wondering if there's anything that could trigger DecrementConnectionCount itself to throw, in which case the above DCC() call could end up throwing, entering this catch block, and calling DCC() again. Maybe we should refactor this try/catch to only have the CreateHttp11ConnectionAsync call in the try/catch?
There was a problem hiding this comment.
DecrementConnectionCount shouldn't throw. That said, I agree that it's better to restructure the code as you suggest. I'll revisit this.
| private ValueTask<HttpConnection> GetOrReserveHttp11ConnectionAsync(CancellationToken cancellationToken) | ||
| { | ||
| if (cancellationToken.IsCancellationRequested) | ||
| { |
There was a problem hiding this comment.
You removed the tracing on purpose? That's fine, just verifying.
There was a problem hiding this comment.
Yes, on purpose. One thing I've noticed in using tracing recently is that it's too verbose about trivial things, e.g. objects getting created. This trace seemed rather trivial, so I removed it.
That said, looking at this particular instance, this is not really an example of unnecessary verbosity; I may have been too eager here.
|
@dotnet-bot test this please |
…, and refactor some connection creation logic
stephentoub
force-pushed
the
morepoollock
branch
from
d620fac to
6b76586
Compare
|
@dotnet-bot test NETFX x86 Release Build |
|
@dotnet-bot test Outerloop Windows x64 Debug Build please |
1 similar comment
|
@dotnet-bot test Outerloop Windows x64 Debug Build please |
…, and refactor some connection creation logic (dotnet/corefx#32524) Commit migrated from dotnet/corefx@83e8985
4 participants
This change moves the connection initiation outside of the pool lock.
Also, refactor slightly so that there is a single path for new connection creation, whether this happens because we are below the connection limit, or when the waiter completes with an available connection count but no actual connection.
@stephentoub