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.
| @@ -234,15 +234,14 @@ private static uint HandleEventConnected(State state, ref ConnectionEvent connec | |||
|
|
|||
| private static uint HandleEventShutdownInitiatedByTransport(State state, ref ConnectionEvent connectionEvent) | |||
| { | |||
| if (!state.Connected && state.ConnectTcs != null) | |||
| if (!state.Connected && state.ConnectTcs != null && !state.ConnectTcs.Task.IsCompleted) | |||
| { | |||
| Debug.Assert(state.Connection != null); | |||
| state.Connection = null; | |||
|
|
|||
| 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
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