-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[HTTP/3] Fix NullReferenceException on cancellation #54334
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.
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
Changes from 1 commit
bef50e2
53d52a5
6f67cae
9bca2f4
88cfca5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…dd test
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -82,8 +82,9 @@ public void Dispose() | |||||||||||||||||||
| if (!_disposed) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| _disposed = true; | ||||||||||||||||||||
| _stream.Dispose(); | ||||||||||||||||||||
| DisposeSyncHelper(); | ||||||||||||||||||||
| var stream = Interlocked.Exchange(ref _stream, null!); | ||||||||||||||||||||
| stream.Dispose(); | ||||||||||||||||||||
| DisposeSyncHelper(stream); | ||||||||||||||||||||
|
Comment on lines
+84
to
+86
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -92,16 +93,15 @@ public async ValueTask DisposeAsync() | |||||||||||||||||||
| if (!_disposed) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| _disposed = true; | ||||||||||||||||||||
| await _stream.DisposeAsync().ConfigureAwait(false); | ||||||||||||||||||||
| DisposeSyncHelper(); | ||||||||||||||||||||
| var stream = Interlocked.Exchange(ref _stream, null!); | ||||||||||||||||||||
| await stream.DisposeAsync().ConfigureAwait(false); | ||||||||||||||||||||
| DisposeSyncHelper(stream); | ||||||||||||||||||||
|
Comment on lines
+95
to
+97
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private void DisposeSyncHelper() | ||||||||||||||||||||
| private void DisposeSyncHelper(QuicStream stream) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| _connection.RemoveStream(_stream); | ||||||||||||||||||||
| _connection = null!; | ||||||||||||||||||||
| _stream = null!; | ||||||||||||||||||||
| Interlocked.Exchange(ref _connection, null!).RemoveStream(stream); | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we've got sole ownership of the stream now due to the Interlocked in the dispose methods above, do we still need this interlocked? Or is there another caller that could get here outside of Dispose{Async}? Also, if it is still needed, then presumably there's a race condition around nulling this out, in which case the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've mostly done that to be sure the change is visible to null checks in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest simply not nulling these fields out (i.e. I'm not quite sure why you null them out today. Is it because you want to avoid doing stuff like Abort on the connection or AbortRead on the QuicStream if we are disposed? Because as the code stands, this is all racy and you aren't avoiding these calls consistently. You need to either: |
||||||||||||||||||||
|
|
||||||||||||||||||||
| _sendBuffer.Dispose(); | ||||||||||||||||||||
| _recvBuffer.Dispose(); | ||||||||||||||||||||
|
|
@@ -1111,18 +1111,26 @@ private void HandleReadResponseContentException(Exception ex, CancellationToken | |||||||||||||||||||
| throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, abortException)); | ||||||||||||||||||||
| case Http3ConnectionException _: | ||||||||||||||||||||
| // A connection-level protocol error has occurred on our stream. | ||||||||||||||||||||
| _connection.Abort(ex); | ||||||||||||||||||||
| _connection?.Abort(ex); | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When can this happen? It seems weird that errors on a single HTTP stream would cause the whole connection to be killed. |
||||||||||||||||||||
| throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, ex)); | ||||||||||||||||||||
| case OperationCanceledException oce when oce.CancellationToken == cancellationToken: | ||||||||||||||||||||
| _stream.AbortWrite((long)Http3ErrorCode.RequestCancelled); | ||||||||||||||||||||
| _stream?.AbortRead((long)Http3ErrorCode.RequestCancelled); | ||||||||||||||||||||
| ExceptionDispatchInfo.Throw(ex); // Rethrow. | ||||||||||||||||||||
| return; // Never reached. | ||||||||||||||||||||
| default: | ||||||||||||||||||||
| _stream.AbortWrite((long)Http3ErrorCode.InternalError); | ||||||||||||||||||||
| _stream?.AbortRead((long)Http3ErrorCode.InternalError); | ||||||||||||||||||||
| throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, ex)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private void CancelResponseContentRead() | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if (_responseDataPayloadRemaining != -1) // -1 indicates EOS | ||||||||||||||||||||
| { | ||||||||||||||||||||
| _stream.AbortRead((long)Http3ErrorCode.RequestCancelled); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private async ValueTask<bool> ReadNextDataFrameAsync(HttpResponseMessage response, CancellationToken cancellationToken) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if (_responseDataPayloadRemaining == -1) | ||||||||||||||||||||
|
|
@@ -1194,6 +1202,7 @@ protected override void Dispose(bool disposing) | |||||||||||||||||||
| { | ||||||||||||||||||||
| if (disposing) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| _stream.CancelResponseContentRead(); | ||||||||||||||||||||
| // This will remove the stream from the connection properly. | ||||||||||||||||||||
| _stream.Dispose(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
@@ -1216,6 +1225,7 @@ public override async ValueTask DisposeAsync() | |||||||||||||||||||
| { | ||||||||||||||||||||
| if (_stream != null) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| _stream.CancelResponseContentRead(); | ||||||||||||||||||||
| await _stream.DisposeAsync().ConfigureAwait(false); | ||||||||||||||||||||
| _stream = null!; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need _disposed, or does _stream being null now indicate disposal?