[S.N.Quic] Observe exceptions in ResettableValueTaskSource #114226
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a design concern with exception handling in QUIC streams by ensuring that exceptions are observed and do not leak, while simplifying the API of the ResettableValueTaskSource.
- Removed the redundant "final" parameter from TrySetException calls in QuicStream.
- Updated ResettableValueTaskSource to always treat exception completion as final.
- Added explicit observation of exceptions in the final task to prevent unobserved exceptions.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs | Removed the now-unnecessary "final" parameter in TrySetException calls, simplifying exception propagation. |
| src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs | Refactored TrySetException to always complete in a final state and added explicit observation of exception tasks. |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Show resolved
Hide resolved
|
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
Outdated
Show resolved
Hide resolved
| /// <param name="exception">The exception to set as a result of the value task.</param> | ||
| /// <returns><c>true</c> if this is the first call that set the result; otherwise, <c>false</c>.</returns> | ||
| public bool TrySetException(Exception exception, bool final = false) | ||
| public bool TrySetException(Exception exception) |
There was a problem hiding this comment.
Just for the explanation: the bool final = false was here to have the "same" signature as TrySetResult and in the past we did have non-final exceptions (from cancellation).
It could have also gone the other way by making final mandatory to all calls to TrySetResult and TrySetException.
Anyway, this is not a hill I want to die on, so keep the change as-is if you prefer it this way.
There was a problem hiding this comment.
I don't plan to do further refactors in this PR, just leaving my take: It would be better to make it mandatory on TrySetResult() so it's easier to see what the call sites are doing. We can add it back to TrySetException when/if it turns out to be needed again, till then I don't see the value.
|
/azp run runtime-libraries stress-http |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ettableValueTaskSource.cs Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There are use cases of
QuicStream.WritesClosed/QuicStream.ReadsClosedin .NET itself which only query theIsCompletedSuccessfullyproperty:The current implementation would require those call sites to await the task or do the
_ = task.Exceptiontrick to avoid leaking unobserved exceptions. IMO this behavior is hard to communicate and the fact that we "misused" our own API is telling. It seems more reasonable if the implementation ensured that exceptions from those tasks are never forwarded toTaskScheduler.UnobservedTaskException.This PR implements this behavior in
ResettableValueTaskSource. Beyond that, it includes a refactor of theTrySetException()method removing itsfinalparameter since it was never called withfinal: false, which felt confusing when working with the code.Fixes #114128.
Running #114225 against this PR locally reduced the number of unobserved exceptions to zero for me.