Call DisposeAsync on SocketsHttpHandler HttpConnection Stream #114339
Conversation
ghost
added
the
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs:114
- Using GetAwaiter().GetResult() here may lead to blocking if the DisposeAsync ValueTask is not already completed. If the underlying stream might perform asynchronous operations, consider ensuring it returns an already-completed ValueTask or document why blocking here is safe.
_stream.DisposeAsync().AsTask().GetAwaiter().GetResult();
|
Tagging subscribers to this area: @dotnet/ncl |
| { | ||
| GC.SuppressFinalize(this); | ||
| _stream.Dispose(); | ||
| _stream.DisposeAsync().AsTask().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
If this makes a difference, that suggests there's a Stream implementation being used that has additional functionality in its DisposeAsync that it doesn't have in its Dispose, sync vs async aside. Isn't the root of the issue here that the stream in question is overriding DisposeAsync but not Dispose?
There was a problem hiding this comment.
Isn't the root of the issue here that the stream in question is overriding DisposeAsync but not Dispose?
It is, but shouldn't you be allowed to override DisposeAsync instead of Dispose?
Most of the time, this should return an already-completed ValueTask, and not make much of a difference, but it can make a difference for someone using the ConnectCallback with a Stream type that implements DisposeAsync instead of Dispose(bool).
You can take a look at modelcontextprotocol/csharp-sdk#225 to see the conversation when I first noticed that SocketsHttpHandler wasn't calling DisposeAsync. If it had, it would have saved me some debugging.
There was a problem hiding this comment.
It is, but shouldn't you be allowed to override DisposeAsync instead of Dispose?
It's very unintuitive if Dispose and DisposeAsync have different functionality, just as it's very unintuitive if Read/ReadAsync, Write/WriteAsync, etc., have different functionality that's separate from the "are operations performed synchronously or asynchronously" aspect.
There was a problem hiding this comment.
The proposed change also means that, if there was an operation that could be performed using synchronous I/O, it's now being forced into an async-over-sync path. Before, if it could avoid the sync-over-async in the Dispose implementation, it would.
There was a problem hiding this comment.
Fair enough. I figured it'd be nicer to lazy Stream implementors like me to Have HttpConnection call DisposeAsync instead, since that also calls Dispose (bool) as long as you call base.DisposeAsync(), but I do see how that could be a perf trap given the sync-over-async. Not that synchronously blocking would be amazing either, but it might be one less thread per call if Dispose is implemented without doing sync-over-async itself.
It would be even better if HttpClient and SocketsHttpHandler implemented IAsyncDisposable, but that would be a far more drastic change.
2 participants
Most of the time, this should return an already-completed ValueTask, and not make much of a difference, but it can make a difference for someone using the ConnectCallback with a Stream type that implements DisposeAsync instead of Dispose(bool).
You can take a look at modelcontextprotocol/csharp-sdk#225 to see the conversation when I first noticed that SocketsHttpHandler wasn't calling DisposeAsync. If it had, it would have saved me some debugging.