avoid NRE in MsQuicConnection ConnectAsync #56283
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis is recent regression caused by #55468. fixes #56263
|
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
Outdated
Show resolved
Hide resolved
| state.ConnectTcs.SetException(ex); | ||
| state.ConnectTcs = null; | ||
| // This is opportunistic if we get exception and have ability to propagate it to caller. | ||
| state.ConnectTcs.TrySetException(ex); |
There was a problem hiding this comment.
This is the change I was hinting at when asking about the race condition.
There was a problem hiding this comment.
I think I can do better fix. Instead of checking state of the task I can simply store TaskCompletionSource in ConnectAsync to local variable and return that. There is really no reason to drag it past Connect event.
|
|
||
| uint hresult = connectionEvent.Data.ShutdownInitiatedByTransport.Status; | ||
| Exception ex = QuicExceptionHelpers.CreateExceptionForHResult(hresult, "Connection has been shutdown by transport."); | ||
| state.ConnectTcs!.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(ex)); |
There was a problem hiding this comment.
This doesn't need to be TrySetException as well?
|
I did different fix @stephentoub, can you please take another quic look. To avoid fighting over global state, I store the original tcs in local variable so it would never NRE. And then the |
src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
Show resolved
Hide resolved
ghost
locked as resolved and limited conversation to collaborators
4 participants
This is recent regression caused by #55468.
When we fail faster than we can return
ConnectTcsinConnectAsync, it may be null and the return would fail.So instead of setting it to null to prefer double cleanup I added
Task.IsCompletedcheck.fixes #56263
fixes #56454