[QUIC] Throw on write after closed writing side#128494
Open
ManickaP wants to merge 2 commits into
Open
Conversation
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts System.Net.Quic.QuicStream.WriteAsync to throw when callers attempt to write after the stream’s writing side has been gracefully completed, and adds a functional regression test to lock in the behavior. It also includes minor doc-comment grammar fixes and adds a new localized error string.
Changes:
- Update
QuicStream.WriteAsyncto throwInvalidOperationExceptionwhen a previously returned writeValueTaskis already completed successfully (intended to represent a closed writing side). - Add a new functional test validating
WriteAsyncthrows afterCompleteWrites()+WritesClosed. - Fix small “is/if” typos in internal task source XML docs and add a new resx string for the new exception message.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs | Changes WriteAsync behavior for post-write-closure calls to throw instead of returning a successful completed ValueTask. |
| src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs | Adds regression coverage for writing after the writing side has closed. |
| src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ValueTaskSource.cs | Fixes XML doc grammar (“is” → “if”). |
| src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs | Fixes XML doc grammar (“is” → “if”). |
| src/libraries/System.Net.Quic/src/Resources/Strings.resx | Adds a new localized string for the new InvalidOperationException message. |
Comment on lines
+423
to
+425
| // It doesn't matter that we throw away the valueTask here, it doesn't need to be reset anymore. | ||
| // The writing side is closed, the task is completed, and it will never get past this condition. | ||
| return valueTask.IsCompletedSuccessfully ? |
Comment on lines
+800
to
+802
| // These both should throw the same exception. | ||
| await Assert.ThrowsAsync<InvalidOperationException>(async () => await clientStream.WriteAsync(new byte[0], false)); | ||
| await Assert.ThrowsAsync<InvalidOperationException>(async () => await clientStream.WriteAsync(new byte[0], false)); |
| <value>Authentication failed because the remote party sent a TLS alert: '{0}'.</value> | ||
| </data> | ||
| <data name="net_writecompleted_invalidcall" xml:space="preserve"> | ||
| <value>This method may not be called when writing side was already completed.</value> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #121619