Skip to content
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

Fix a race condition cause by out of band signaling #212

Merged
merged 1 commit into from
May 3, 2024

Conversation

mengelbart
Copy link
Owner

Signaling flow IDs may happen out-of-band and cause a race condition when flow IDs are received before signaling is complete.

The solution is modeled after WebTransport's handling of incoming streams and datagrams described in https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3-09#section-4.5

Copy link
Collaborator

@SpencerDawkins SpencerDawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly concerned that we're still finding "QUIC datagram(s)" in the text - I thought I'd upshifted all of those in a previous PR. Maybe I missed some?

@@ -518,7 +518,20 @@ the same flow identifier (following the procedures defined in {{?RFC5761}}), or
they can use different flow identifiers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments below assume that buffering RoQ streams and RoQ datagrams carrying real-time RTP payloads is the Right Thing To Do. We can talk about whether that's desirable, because buffering real-time RTP will cause the RTP session to start out with an overestimated RTT and will introduce jitter as the RTP sender responds to the actual RTT after the RTP receiver starts to provide feedback, but we should at least think about that, because RTP is designed to tolerate packet loss, so admitting a burst of RTP packets to an application when the receiver finally knows what to do with the flow identifier might not be our best option.

My comments also assume that some RoQ receivers might buffer RoQ streams and datagrams while signaling completes, and others might not, so I introduced "buffering endpoint", to make that distinction.

@@ -518,7 +518,20 @@ the same flow identifier (following the procedures defined in {{?RFC5761}}), or
they can use different flow identifiers.

The association between flow identifiers and RTP streams MUST be negotiated
using appropriate signaling. If a receiver cannot associate a flow identifier
using appropriate signaling. The signaling happens out of band and thus a stream
or datagram with a given flow identifer may arrive before the signaling
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
or datagram with a given flow identifer may arrive before the signaling
or datagram with a given flow identifier can arrive before the signaling

using appropriate signaling. If a receiver cannot associate a flow identifier
using appropriate signaling. The signaling happens out of band and thus a stream
or datagram with a given flow identifer may arrive before the signaling
finished. In that case, an endpoint may not be able to associate the stream or
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
finished. In that case, an endpoint may not be able to associate the stream or
finished. In that case, an endpoint cannot associate the stream or

using appropriate signaling. The signaling happens out of band and thus a stream
or datagram with a given flow identifer may arrive before the signaling
finished. In that case, an endpoint may not be able to associate the stream or
datagram with the corresponding RTP stream. The endpoint SHOULD buffer streams
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
datagram with the corresponding RTP stream. The endpoint SHOULD buffer streams
datagram with the corresponding RTP stream. The endpoint can buffer streams

finished. In that case, an endpoint may not be able to associate the stream or
datagram with the corresponding RTP stream. The endpoint SHOULD buffer streams
and datagrams using an unknown flow identifier until they can be associated with
the corresponding RTP stream. To avoid resource exhaustion, the endpoint SHOULD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
the corresponding RTP stream. To avoid resource exhaustion, the endpoint SHOULD
the corresponding RTP stream. To avoid resource exhaustion, the buffering endpoint MUST

For this to be a normative SHOULD, we should have some situation in mind where it makes sense for a receiver to buffer streams and datagrams without setting a limit. Do we have such a situation in mind?

and datagrams using an unknown flow identifier until they can be associated with
the corresponding RTP stream. To avoid resource exhaustion, the endpoint SHOULD
limit the number of streams and datagrams to buffer. If the number of buffered
streams exceeds the limit, the endpoint SHOULD send a STOP_SENDING with the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
streams exceeds the limit, the endpoint SHOULD send a STOP_SENDING with the
streams exceeds the limit on buffered streams, the endpoint MUST send a STOP_SENDING with the

limit the number of streams and datagrams to buffer. If the number of buffered
streams exceeds the limit, the endpoint SHOULD send a STOP_SENDING with the
error code ROQ_UNKNOWN_FLOW_ID. It is an implementation's choice on which stream
to send STOP_SENDING. If the number of buffered datagrams exceeds the limit, the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to send STOP_SENDING. If the number of buffered datagrams exceeds the limit, the
to send STOP_SENDING. If the number of buffered datagrams exceeds the limit on buffered datagrams, the

streams exceeds the limit, the endpoint SHOULD send a STOP_SENDING with the
error code ROQ_UNKNOWN_FLOW_ID. It is an implementation's choice on which stream
to send STOP_SENDING. If the number of buffered datagrams exceeds the limit, the
endpoint SHOULD drop a datagram. It is an implementation's choice which datagram
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
endpoint SHOULD drop a datagram. It is an implementation's choice which datagram
endpoint MUST drop a datagram. It is an implementation's choice which datagram

@mengelbart mengelbart force-pushed the fix/unknown-flow-id-race-condition branch 2 times, most recently from 669407a to 6197bf2 Compare May 3, 2024 10:34
Signaling flow IDs may happen out of band and cause a race condition
when flow IDs are received before signaling is complete.
@mengelbart mengelbart force-pushed the fix/unknown-flow-id-race-condition branch from 6197bf2 to 0ad8e29 Compare May 3, 2024 10:36
@mengelbart
Copy link
Owner Author

I'm slightly concerned that we're still finding "QUIC datagram(s)" in the text - I thought I'd upshifted all of those in a previous PR. Maybe I missed some?

I think I introduced some new ones in this PR. I updated it and also applied the other suggestions you made.

@mengelbart mengelbart merged commit 66d7e8d into main May 3, 2024
2 checks passed
@mengelbart mengelbart deleted the fix/unknown-flow-id-race-condition branch May 3, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants