Fix reentrant assert failures in session resumption with custom executions#5844
Fix reentrant assert failures in session resumption with custom executions#5844
Conversation
…cutions When MsQuicLib.CustomExecutions is true, SetParam calls execute inline with InlineApiExecution = TRUE. For QUIC_PARAM_CONN_RESUMPTION_TICKET, the call chain reaches QuicStreamSetIndicateStreamsAvailable -> QuicConnIndicateEvent, Since the resumption ticket is set before ConnectionStart, there are no streams yet, so the STREAMS_AVAILABLE event is meaningless. Gate the indication on FlushIfUnblocked (which is already FALSE for the resumption ticket path), consistent with the existing flush guard.
leikong
changed the title
leikong
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5844 +/- ##
==========================================
- Coverage 86.24% 85.63% -0.61%
==========================================
Files 60 60
Lines 18732 18732
==========================================
- Hits 16156 16042 -114
- Misses 2576 2690 +114 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
I am not sure we can simply skip those notifications.
With 0-RTT, it should be possible for the app to queue streams and datagram before the connection is started, and it seems reasonable to me the app would rely on STREAM_AVAIALBLE and DATAGRAM_STATE_CHANGED notification to know what to queue as a reaction to setting the resumption ticket.
The assert is meant to avoid re-entrant notifications. In your scenario, the problem is not a re-entrant notification AFAIU. Can we relax the assertion instead? Or is the assertion reporting a real issue here?
The following are the two assert failure stacks, I don't see callback reentrancy, please also verify, thanks. Stack with QuicStreamSetIndicateStreamsAvailable: Stack with QuicDatagramOnSendStateChanged: |
…essing Instead of skipping STREAMS_AVAILABLE and DATAGRAM_STATE_CHANGED notifications when called from the resumption ticket path (which breaks 0-RTT scenarios), defer them by queuing operations that get processed when the worker drains the operation queue, outside the InlineApiExecution context. - Add QUIC_OPER_TYPE_STREAMS_AVAILABLE and QUIC_OPER_TYPE_DATAGRAM_STATE_CHANGED operation types. instead of calling QuicStreamSetIndicateStreamsAvailable directly. - When FromResumptionTicket, enqueue DATAGRAM_STATE_CHANGED instead of calling QuicDatagramOnSendStateChanged directly. - Handle both new operation types in QuicConnDrainOperations. - Make QuicSendQueueFlush unconditional since it only enqueues a FLUSH_SEND operation and does not trigger app callbacks. - Export QuicStreamSetIndicateStreamsAvailable declaration in stream_set.h.
I think we are saying the same thing. The original intent of the assert is to avoid reentrancy when in notification handling. My point is that we should consider relaxing the assertion without changing the logic unless we have a clear issue to justify the behavior change. |
I have made the change to queue the notifications when |
…bugcheck.in.sesstion.resumption
|
Sorry for the delay.
I think we should relax the assertion to: unless there is an actual issue you are hitting on release builds currently. I don't think changing the behavior when custom execution is not used only because of a debug assert hit with custom execution enabled is acceptable. On a second time, we can re-think the design of custom executions if needed (maybe have an async version of get/set param, or another pattern to deal with the category of issues). |
Updated the assert per suggestion, please review again, thanks. |
|
nit: There is a similar assertion in |
I haven't run into it so far, but will keep it in mind, thanks for the info. |
For consistency and to avoid potential confusion in future, I decided to also skip reentrancy assert check for custom execution in QuicStreamIndicateEvent, although this was not encountered in my testing. Also, InlineApiExecution is only read in the two assertions, so technically, we can remove the code that sets InlineApiExecution because of CustomExecutions, but that is a bigger change that affects more code paths, so it's better to err on the safe side to keep the fix simple. |
2 participants
When
MsQuicLib.CustomExecutionsis enabled, allMsQuicSetParamcalls execute inline withConnection->State.InlineApiExecution = TRUE(seeapi.c:1666). ForQUIC_PARAM_CONN_RESUMPTION_TICKET, this triggers a debug assertion failure atconnection.c:693:The assertion fires because
QuicConnIndicateEventis called whileInlineApiExecutionisTRUEandHandleClosedisFALSE. Two distinct call paths trigger this:Path 1: Via streams-available indication
Path 2: Via datagram state change
Fix
Relax the assertion to not check app callback reentrancy when custom execution is enabled, per suggestion by @guhetier.