TL;DR: we fixed #56129 for streams being properly disposed, but it's still there for streams thrown away for finalization.
While working on #58234 I found out that for the finalization test, instead of H3_REQUEST_CANCELLED, I always receive the default S.N.Quic disposal error code -- which means that Http3RequestStream aborting logic was not executed.
It turns out we don't have a finalizer for Http3RequestStream, only Dispose:
|
public void Dispose() |
|
{ |
|
if (!_disposed) |
|
{ |
|
_disposed = true; |
|
AbortStream(); |
|
_stream.Dispose(); |
|
DisposeSyncHelper(); |
|
} |
|
} |
|
|
|
public async ValueTask DisposeAsync() |
|
{ |
|
if (!_disposed) |
|
{ |
|
_disposed = true; |
|
AbortStream(); |
|
await _stream.DisposeAsync().ConfigureAwait(false); |
|
DisposeSyncHelper(); |
|
} |
|
} |
Http3ReadStream has a finalizer, but it doesn't call Dispose for Http3RequestStream:
|
~Http3ReadStream() |
|
{ |
|
Dispose(false); |
|
} |
|
|
|
protected override void Dispose(bool disposing) |
|
{ |
|
Http3RequestStream? stream = Interlocked.Exchange(ref _stream, null); |
|
if (stream is null) |
|
{ |
|
return; |
|
} |
|
|
|
if (disposing) |
|
{ |
|
// This will remove the stream from the connection properly. |
|
stream.Dispose(); |
|
} |
|
else |
|
{ |
|
// We shouldn't be using a managed instance here, but don't have much choice -- we |
|
// need to remove the stream from the connection's GOAWAY collection. |
|
stream._connection.RemoveStream(stream._stream); |
|
stream._connection = null!; |
|
} |
|
|
|
_response = null; |
|
|
|
base.Dispose(disposing); |
|
} |
Http3WriteStream doesn't have a finalizer.
It all means that if Http3RequestStream/Http3ReadStream/Http3WriteStream are left for finalization, only default S.N.Quic disposal logic is executed, which:
- will abort read with 0xffffffff -- not that harmful
- will gracefully close writes -- peer will receive EOF instead of Abort, which may be harmful (corrupted data)
Note that for HTTP/2 we have finalization logic with the following comment
|
protected override void Dispose(bool disposing) |
|
{ |
|
Http2Stream? http2Stream = Interlocked.Exchange(ref _http2Stream, null); |
|
if (http2Stream == null) |
|
{ |
|
return; |
|
} |
|
|
|
// Technically we shouldn't be doing the following work when disposing == false, |
|
// as the following work relies on other finalizable objects. But given the HTTP/2 |
|
// protocol, we have little choice: if someone drops the Http2ReadStream without |
|
// disposing of it, we need to a) signal to the server that the stream is being |
|
// canceled, and b) clean up the associated state in the Http2Connection. |
|
|
|
http2Stream.CloseResponseBody(); |
|
|
|
base.Dispose(disposing); |
|
} |
We should also execute proper aborting logic for Http3RequestStream if it's finalized.