Skip to content

Call DisposeAsync on SocketsHttpHandler HttpConnection Stream #114339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Call DisposeAsync on SocketsHttpHandler HttpConnection Stream #114339

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
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ private void Dispose(bool disposing)
if (disposing)
{
GC.SuppressFinalize(this);
_stream.Dispose();
_stream.DisposeAsync().AsTask().GetAwaiter().GetResult();
Copy link
Member

@stephentoub stephentoub Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@halter73 halter73 Apr 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@stephentoub stephentoub Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@stephentoub stephentoub Apr 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@halter73 halter73 Apr 7, 2025

Choose a reason for hiding this comment

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

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.

}
}
}
Expand Down
99 changes: 99 additions & 0 deletions src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1428,6 +1428,105 @@ public async Task Expect100ContinueTimeout_SetAfterUse_Throws()
Assert.Throws<InvalidOperationException>(() => handler.Expect100ContinueTimeout = TimeSpan.FromMilliseconds(1));
}
}

[Fact]
public async Task ReadBodyCanceled_CallsDisposeAsyncOnConnectCallbackStream()
{
HalfDuplexStream? stream = null;

using var handler = new SocketsHttpHandler();
handler.ConnectCallback = async (context, cancellation) =>
{
Assert.Null(stream);

var serverToClientPipe = new IO.Pipelines.Pipe();
stream = new HalfDuplexStream(serverToClientPipe);

var hangingChunkedResponse = "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n"u8;
var writeSpan = serverToClientPipe.Writer.GetSpan(hangingChunkedResponse.Length);
hangingChunkedResponse.CopyTo(writeSpan);
serverToClientPipe.Writer.Advance(hangingChunkedResponse.Length);
await serverToClientPipe.Writer.FlushAsync(cancellation);
return stream;
};
using HttpClient client = CreateHttpClient(handler);

using HttpRequestMessage request = CreateRequest(HttpMethod.Get, new Uri("http://example.com"), UseVersion, exactVersion: true);
using var response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead).WaitAsync(TestHelper.PassingTestTimeout);
response.EnsureSuccessStatusCode();

using var cts = new CancellationTokenSource();
var responseBodyReadTask = response.Content.ReadAsStringAsync(cts.Token);
cts.Cancel();

var tcException = await Assert.ThrowsAsync<TaskCanceledException>(() => responseBodyReadTask).WaitAsync(TestHelper.PassingTestTimeout);
var ioException = Assert.IsType<HttpIOException>(tcException.InnerException);
Assert.Equal(HttpRequestError.ResponseEnded, ioException.HttpRequestError);

Assert.NotNull(stream);
Assert.True(stream.DisposeCalled);
Assert.True(stream.DisposeAsyncCalled);
}

private class HalfDuplexStream(IO.Pipelines.Pipe responsePipe) : Stream
{
private readonly Stream _readStream = responsePipe.Reader.AsStream();

public bool DisposeCalled { get; private set; }
public bool DisposeAsyncCalled { get; private set; }

public override bool CanRead => true;
public override bool CanWrite => true;
public override bool CanSeek => false;

public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
=> _readStream.ReadAsync(buffer, offset, count, cancellationToken);
public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
=> _readStream.ReadAsync(buffer, cancellationToken);

public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
=> Null.WriteAsync(buffer, offset, count, cancellationToken);
public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)
=> Null.WriteAsync(buffer, cancellationToken);

public override Task FlushAsync(CancellationToken cancellationToken)
=> Null.FlushAsync(cancellationToken);

public override int Read(byte[] buffer, int offset, int count) => _readStream.Read(buffer, offset, count);
public override void Write(byte[] buffer, int offset, int count) => Null.Write(buffer, offset, count);
public override void Flush() => Null.Flush();

protected override void Dispose(bool disposing)
{
DisposeCalled = true;
responsePipe.Writer.Complete();
}

public override async ValueTask DisposeAsync()
{
DisposeAsyncCalled = true;
await base.DisposeAsync();
}

// Unsupported stuff
public override long Length => throw new NotSupportedException();

public override long Position
{
get => throw new NotSupportedException();
set => throw new NotSupportedException();
}

public override long Seek(long offset, SeekOrigin origin)
{
throw new NotSupportedException();
}

public override void SetLength(long value)
{
throw new NotSupportedException();
}
}
}

public abstract class SocketsHttpHandler_HttpClientHandler_MaxResponseHeadersLength : HttpClientHandler_MaxResponseHeadersLength_Test
Expand Down
Loading