-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QUIC: Remove waiting on start event in send path #55442
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #55302 Currently, we are waiting to receive the START_COMPLETED event in HandleWriteStartState. This is unnecessary. Worse, it is causing exceptions during callback event processing, because we have completed SendResettableCompletionSource for the START_COMPLETED event but haven't consumed it, then we try to complete it again when we receive SHUTDOWN_COMPLETE or similar events. Remove the wait for START_COMPLETED and the associated logic to signal it in the event handling code. Enable the assert about not throwing during event processing. Minor other cleanup.
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #55302 Currently, we are waiting to receive the START_COMPLETED event in HandleWriteStartState. This is unnecessary. Worse, it is causing exceptions during callback event processing, because we have completed SendResettableCompletionSource for the START_COMPLETED event but haven't consumed it, then we try to complete it again when we receive SHUTDOWN_COMPLETE or similar events. Remove the wait for START_COMPLETED and the associated logic to signal it in the event handling code. Enable the assert about not throwing during event processing. Minor other cleanup.
|
Do we know for sure it is OK to send/receive before getting START_COMPLETED? |
@@ -831,22 +818,10 @@ private static uint HandleEventPeerRecvAborted(State state, ref StreamEvent evt) | |||
return MsQuicStatusCodes.Success; | |||
} | |||
|
|||
private static uint HandleEventStartComplete(State state) | |||
private static uint HandleEventStartComplete(State state, ref StreamEvent evt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to pass in the event if we are not using it? (now?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to. I added it because I thought I was going to use it, but since we don't have a definition for the START_COMPLETE event data, we can't right now. I don't think there's any harm in having it, though.
// This is getting hit currently | ||
// See https://github.com/dotnet/runtime/issues/55302 | ||
//Debug.Fail($"[Stream#{state.GetHashCode()}] Exception occurred during handling {(QUIC_STREAM_EVENT_TYPE)evt.Type} event: {ex}"); | ||
Debug.Fail($"[Stream#{state.GetHashCode()}] Exception occurred during handling {evt.Type} event: {ex}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should find better way how to propagate exceptions to the caller. This is going to be hard IMHO to debug in CI and does nothing to diagnose filed issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it's a bit of a pain to deal with this.
That said, it's not obvious to me how to propagate this to the caller and/or make it easier to debug. Suggestions welcome.
Hopefully this is not a common occurrence -- an exception during a callback means something is deeply, deeply wrong.
That's how I read the msquic docs, yes. @nibanks Can you confirm? |
You can send at any time. All it does it queue if not started. |
BTW is there limit how much we would buffer? I'm wondering if we could create situation when caller think we are writing to the peer but we indeed simply create internal buffer. |
Yes, there's a limit to the send buffer. Roughly speaking, it works like winsock. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We shall see how the Debug.Fail holds.
Fixes #55302
Currently, we are waiting to receive the START_COMPLETED event in HandleWriteStartState. This is unnecessary. Worse, it is causing exceptions during callback event processing, because we have completed SendResettableCompletionSource for the START_COMPLETED event but haven't consumed it, then we try to complete it again when we receive SHUTDOWN_COMPLETE or similar events.
Remove the wait for START_COMPLETED and the associated logic to signal it in the event handling code.
Enable the assert about not throwing during event processing.
Minor other cleanup.