From 0a4b840622c1bdb36883a7e2c69cf90185712204 Mon Sep 17 00:00:00 2001 From: Nick Banks Date: Fri, 9 Jul 2021 09:52:28 -0700 Subject: [PATCH 1/2] Improve Event Callback Reentrancy --- src/core/api.c | 118 ++++++++++++++++++++++-------------- src/core/connection.c | 44 ++++++++------ src/core/connection.h | 14 +++-- src/core/stream.c | 17 +++++- src/plugins/dbg/quictypes.h | 12 +++- 5 files changed, 128 insertions(+), 77 deletions(-) diff --git a/src/core/api.c b/src/core/api.c index 8cd97b562e..a79faa7d69 100644 --- a/src/core/api.c +++ b/src/core/api.c @@ -118,7 +118,14 @@ MsQuicConnectionClose( // // Execute this blocking API call inline if called on the worker thread. // + BOOLEAN AlreadyInline = Connection->State.InlineApiExecution; + if (!AlreadyInline) { + Connection->State.InlineApiExecution = TRUE; + } QuicConnCloseHandle(Connection); + if (!AlreadyInline) { + Connection->State.InlineApiExecution = FALSE; + } } else { @@ -692,7 +699,14 @@ MsQuicStreamClose( // // Execute this blocking API call inline if called on the worker thread. // + BOOLEAN AlreadyInline = Connection->State.InlineApiExecution; + if (!AlreadyInline) { + Connection->State.InlineApiExecution = TRUE; + } QuicStreamClose(Stream); + if (!AlreadyInline) { + Connection->State.InlineApiExecution = FALSE; + } } else { @@ -785,16 +799,7 @@ MsQuicStreamStart( goto Exit; } - if (Connection->WorkerThreadID == CxPlatCurThreadID()) { - - CXPLAT_PASSIVE_CODE(); - - // - // Execute this blocking API call inline if called on the worker thread. - // - Status = QuicStreamStart(Stream, Flags, FALSE); - - } else if (Flags & QUIC_STREAM_START_FLAG_ASYNC) { + if (Flags & QUIC_STREAM_START_FLAG_ASYNC) { QUIC_OPERATION* Oper = QuicOperationAlloc(Connection->Worker, QUIC_OPER_TYPE_API_CALL); @@ -824,6 +829,22 @@ MsQuicStreamStart( QuicConnQueueOper(Connection, Oper); Status = QUIC_STATUS_PENDING; + } else if (Connection->WorkerThreadID == CxPlatCurThreadID()) { + + CXPLAT_PASSIVE_CODE(); + + // + // Execute this blocking API call inline if called on the worker thread. + // + BOOLEAN AlreadyInline = Connection->State.InlineApiExecution; + if (!AlreadyInline) { + Connection->State.InlineApiExecution = TRUE; + } + Status = QuicStreamStart(Stream, Flags, FALSE); + if (!AlreadyInline) { + Connection->State.InlineApiExecution = FALSE; + } + } else { CXPLAT_PASSIVE_CODE(); @@ -926,46 +947,35 @@ MsQuicStreamShutdown( Connection = Stream->Connection; QUIC_CONN_VERIFY(Connection, !Connection->State.Freed); + QUIC_CONN_VERIFY(Connection, !Connection->State.HandleClosed); - if (Connection->WorkerThreadID == CxPlatCurThreadID()) { - // - // Execute this blocking API call inline if called on the worker thread. - // - QuicStreamShutdown(Stream, Flags, ErrorCode); - Status = QUIC_STATUS_SUCCESS; - - } else { - - QUIC_CONN_VERIFY(Connection, !Connection->State.HandleClosed); - - Oper = QuicOperationAlloc(Connection->Worker, QUIC_OPER_TYPE_API_CALL); - if (Oper == NULL) { - Status = QUIC_STATUS_OUT_OF_MEMORY; - QuicTraceEvent( - AllocFailure, - "Allocation of '%s' failed. (%llu bytes)", - "STRM_SHUTDOWN operation", - 0); - goto Error; - } - Oper->API_CALL.Context->Type = QUIC_API_TYPE_STRM_SHUTDOWN; - Oper->API_CALL.Context->STRM_SHUTDOWN.Stream = Stream; - Oper->API_CALL.Context->STRM_SHUTDOWN.Flags = Flags; - Oper->API_CALL.Context->STRM_SHUTDOWN.ErrorCode = ErrorCode; + Oper = QuicOperationAlloc(Connection->Worker, QUIC_OPER_TYPE_API_CALL); + if (Oper == NULL) { + Status = QUIC_STATUS_OUT_OF_MEMORY; + QuicTraceEvent( + AllocFailure, + "Allocation of '%s' failed. (%llu bytes)", + "STRM_SHUTDOWN operation", + 0); + goto Error; + } + Oper->API_CALL.Context->Type = QUIC_API_TYPE_STRM_SHUTDOWN; + Oper->API_CALL.Context->STRM_SHUTDOWN.Stream = Stream; + Oper->API_CALL.Context->STRM_SHUTDOWN.Flags = Flags; + Oper->API_CALL.Context->STRM_SHUTDOWN.ErrorCode = ErrorCode; - // - // Async stream operations need to hold a ref on the stream so that the - // stream isn't freed before the operation can be processed. The ref is - // released after the operation is processed. - // - QuicStreamAddRef(Stream, QUIC_STREAM_REF_OPERATION); + // + // Async stream operations need to hold a ref on the stream so that the + // stream isn't freed before the operation can be processed. The ref is + // released after the operation is processed. + // + QuicStreamAddRef(Stream, QUIC_STREAM_REF_OPERATION); - // - // Queue the operation but don't wait for the completion. - // - QuicConnQueueOper(Connection, Oper); - Status = QUIC_STATUS_PENDING; - } + // + // Queue the operation but don't wait for the completion. + // + QuicConnQueueOper(Connection, Oper); + Status = QUIC_STATUS_PENDING; Error: @@ -1356,7 +1366,14 @@ MsQuicSetParam( // // Execute this blocking API call inline if called on the worker thread. // + BOOLEAN AlreadyInline = Connection->State.InlineApiExecution; + if (!AlreadyInline) { + Connection->State.InlineApiExecution = TRUE; + } Status = QuicLibrarySetParam(Handle, Level, Param, BufferLength, Buffer); + if (!AlreadyInline) { + Connection->State.InlineApiExecution = FALSE; + } goto Error; } @@ -1486,7 +1503,14 @@ MsQuicGetParam( // // Execute this blocking API call inline if called on the worker thread. // + BOOLEAN AlreadyInline = Connection->State.InlineApiExecution; + if (!AlreadyInline) { + Connection->State.InlineApiExecution = TRUE; + } Status = QuicLibraryGetParam(Handle, Level, Param, BufferLength, Buffer); + if (!AlreadyInline) { + Connection->State.InlineApiExecution = FALSE; + } goto Error; } diff --git a/src/core/connection.c b/src/core/connection.c index 2957974b8e..056aa4873f 100644 --- a/src/core/connection.c +++ b/src/core/connection.c @@ -487,6 +487,7 @@ QuicConnCloseHandle( ) { CXPLAT_TEL_ASSERT(!Connection->State.HandleClosed); + Connection->State.HandleClosed = TRUE; QuicConnCloseLocally( Connection, @@ -498,7 +499,6 @@ QuicConnCloseHandle( QuicConnOnShutdownComplete(Connection); } - Connection->State.HandleClosed = TRUE; Connection->ClientCallbackHandler = NULL; if (Connection->State.Registered) { @@ -683,27 +683,32 @@ QuicConnIndicateEvent( ) { QUIC_STATUS Status; - if (!Connection->State.HandleClosed) { - QUIC_CONN_VERIFY(Connection, Connection->State.HandleShutdown || Connection->ClientCallbackHandler != NULL || !Connection->State.ExternalOwner); - if (Connection->ClientCallbackHandler == NULL) { - Status = QUIC_STATUS_INVALID_STATE; - QuicTraceLogConnWarning( - ApiEventNoHandler, - Connection, - "Event silently discarded (no handler)."); - } else { - Status = - Connection->ClientCallbackHandler( - (HQUIC)Connection, - Connection->ClientContext, - Event); - } + if (Connection->ClientCallbackHandler != NULL) { + // + // MsQuic shouldn't indicate reentrancy to the app when at all possible. + // The general exception to this rule is when the connection is being + // closed because the API MUST block until all work is completed, so we + // have to execute the event callbacks inline. + // + CXPLAT_DBG_ASSERT( + !Connection->State.InlineApiExecution || + Connection->State.HandleClosed); + Status = + Connection->ClientCallbackHandler( + (HQUIC)Connection, + Connection->ClientContext, + Event); } else { + QUIC_CONN_VERIFY( + Connection, + Connection->State.HandleClosed || + Connection->State.HandleShutdown || + !Connection->State.ExternalOwner); Status = QUIC_STATUS_INVALID_STATE; QuicTraceLogConnWarning( - ApiEventAlreadyClosed, + ApiEventNoHandler, Connection, - "Event silently discarded."); + "Event silently discarded (no handler)."); } return Status; } @@ -1402,7 +1407,7 @@ QuicConnOnShutdownComplete( Event.SHUTDOWN_COMPLETE.PeerAcknowledgedShutdown = !Connection->State.ShutdownCompleteTimedOut; Event.SHUTDOWN_COMPLETE.AppCloseInProgress = - Connection->State.AppCloseInProgress; + Connection->State.HandleClosed; QuicTraceLogConnVerbose( IndicateConnectionShutdownComplete, @@ -6554,7 +6559,6 @@ QuicConnProcessApiOperation( switch (ApiCtx->Type) { case QUIC_API_TYPE_CONN_CLOSE: - Connection->State.AppCloseInProgress = TRUE; QuicConnCloseHandle(Connection); break; diff --git a/src/core/connection.h b/src/core/connection.h index 3e0380c81f..20cbd76951 100644 --- a/src/core/connection.h +++ b/src/core/connection.h @@ -147,18 +147,18 @@ typedef union QUIC_CONNECTION_STATE { // BOOLEAN ResumptionEnabled : 1; - // - // Indicates that an app close from a non worker thread is in progress. - // Received by the QUIC_CONNECTION_EVENT_SHUTDOWN_COMPLETE event. - // - BOOLEAN AppCloseInProgress: 1; - // // When true, this indicates that reordering shouldn't elict an // immediate acknowledgement. // BOOLEAN IgnoreReordering : 1; + // + // When true, this indicates that the connection is currently executing + // and API call inline (from a reentrant call on a callback). + // + BOOLEAN InlineApiExecution : 1; + #ifdef CxPlatVerifierEnabledByAddr // // The calling app is being verified (app or driver verifier). @@ -168,6 +168,8 @@ typedef union QUIC_CONNECTION_STATE { }; } QUIC_CONNECTION_STATE; +CXPLAT_STATIC_ASSERT(sizeof(QUIC_CONNECTION_STATE) == sizeof(uint32_t), "Ensure correct size/type"); + // // Different references on a connection. // diff --git a/src/core/stream.c b/src/core/stream.c index 7d5d89d7a7..54f29030cb 100644 --- a/src/core/stream.c +++ b/src/core/stream.c @@ -317,6 +317,9 @@ QuicStreamClose( _In_ __drv_freesMem(Mem) QUIC_STREAM* Stream ) { + CXPLAT_DBG_ASSERT(!Stream->Flags.HandleClosed); + Stream->Flags.HandleClosed = TRUE; + if (!Stream->Flags.ShutdownComplete) { if (Stream->Flags.Started) { @@ -352,7 +355,6 @@ QuicStreamClose( } } - Stream->Flags.HandleClosed = TRUE; Stream->ClientCallbackHandler = NULL; QuicStreamRelease(Stream, QUIC_STREAM_REF_APP); @@ -388,6 +390,19 @@ QuicStreamIndicateEvent( { QUIC_STATUS Status; if (Stream->ClientCallbackHandler != NULL) { + // + // MsQuic shouldn't indicate reentrancy to the app when at all + // possible. The general exception to this rule is when the connection + // or stream is being closed because the API MUST block until all work + // is completed, so we have to execute the event callbacks inline. There + // is also one additional exception for start complete when StreamStart + // is called synchronously on an MsQuic thread. + // + CXPLAT_DBG_ASSERT( + !Stream->Connection->State.InlineApiExecution || + Stream->Connection->State.HandleClosed || + Stream->Flags.HandleClosed || + Event->Type == QUIC_STREAM_EVENT_START_COMPLETE); Status = Stream->ClientCallbackHandler( (HQUIC)Stream, diff --git a/src/plugins/dbg/quictypes.h b/src/plugins/dbg/quictypes.h index 2be896084f..882651d337 100644 --- a/src/plugins/dbg/quictypes.h +++ b/src/plugins/dbg/quictypes.h @@ -193,10 +193,16 @@ typedef union QUIC_CONNECTION_STATE { BOOLEAN ResumptionEnabled : 1; // - // Indicates that an app close from a non worker thread is in progress. - // Received by the QUIC_CONNECTION_EVENT_SHUTDOWN_COMPLETE event. + // When true, this indicates that reordering shouldn't elict an + // immediate acknowledgement. // - BOOLEAN AppCloseInProgress: 1; + BOOLEAN IgnoreReordering : 1; + + // + // When true, this indicates that the connection is currently executing + // and API call inline (from a reentrant call on a callback). + // + BOOLEAN InlineApiExecution : 1; #ifdef CxPlatVerifierEnabledByAddr // From b666f72264396ed5721fec33c6bb056f22024fae Mon Sep 17 00:00:00 2001 From: Nick Banks Date: Fri, 9 Jul 2021 10:01:14 -0700 Subject: [PATCH 2/2] Fix typo --- src/core/connection.h | 2 +- src/plugins/dbg/quictypes.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/connection.h b/src/core/connection.h index 20cbd76951..ea496ec949 100644 --- a/src/core/connection.h +++ b/src/core/connection.h @@ -155,7 +155,7 @@ typedef union QUIC_CONNECTION_STATE { // // When true, this indicates that the connection is currently executing - // and API call inline (from a reentrant call on a callback). + // an API call inline (from a reentrant call on a callback). // BOOLEAN InlineApiExecution : 1; diff --git a/src/plugins/dbg/quictypes.h b/src/plugins/dbg/quictypes.h index 882651d337..a2426bbda8 100644 --- a/src/plugins/dbg/quictypes.h +++ b/src/plugins/dbg/quictypes.h @@ -200,7 +200,7 @@ typedef union QUIC_CONNECTION_STATE { // // When true, this indicates that the connection is currently executing - // and API call inline (from a reentrant call on a callback). + // an API call inline (from a reentrant call on a callback). // BOOLEAN InlineApiExecution : 1;