Proto: Ergonomics: Events are emitted before accepting stream

[FlorianUekermann]

Connection::poll emits stream events on remotely opened streams like Event::Stream(StreamEvent::Readable) or Event::Stream(StreamEvent::Stopped) before the application learns about their existence via a call to Connection::streams().accept.

This is a little awkward for application side stream state tracking, because StreamEvent::Opened is not emitted per stream (and therefore lacks stream id information). It would be more ergonomic if there was a reliable first contact point for each incoming stream like a per stream StreamEvent::Opened or Connection::streams().accept where the app learns about the existence of a new stream.

Bit of background why this matters to me: The reason I track each stream state in the first place is that quinn-proto may have forgotten about a stream by the time the application interacts with it and I want accurate errors. The issue above complicates the lifecycle of my stream state information, because I can't just forget stream states as soon as the app dropped the stream handle, but need to keep it around until I know quinn-proto won't emit any more events for it to avoid recreating incomplete stream states if a new event pops up after the app dropped all stream handles.

The contract upheld by quinn-proto in this context isn't very obvious to me. What I have come up with as best guess for guaranteeing I won't see stream events after dropping the tracking info of a bidi streams is this:

• Create stream state tracking info on the first mention of the stream by quinn-proto in return of accept or any event.
• Remove stream state tracking info if:
  • All app side stream handles have been dropped.
  • Recv side is finished or stopped.
  • Send side called finish or reset or remote stopped.
  • In drop of stream handles:
  • Call RecvStream::stop if stream hasn't been finished or stopped.
  • Call SendStream::reset if finish or reset haven't been called or remote stopped.

I could forget about all of this complexity and just drop the state tracking info when the stream handle is dropped if either:

• No stream events are emitted before a stream id was returned by accept.
• The StreamEvent::Opened event was guaranteed to fire once per remotely opened stream, preferably before accept returns the id, but that't not a must.

[FlorianUekermann]

Maybe my issue stems from a misunderstanding of what calling accept implies. I assumed that acknowledgment of that stream to the remote is delayed until accept is called. If that is not the case, and all accept does is communicate the stream id, I may as well drain accept whenever StreamEvent::Opened is emitted. However, then I am quite confused why accept exists in the first place instead of poll emitting a corresponding event.

Documenting the meaning of accept would maybe be a good idea in any case, because the RFC doesn't use that kind of language around streams afaik.

[Ralith]

I assumed that acknowledgment of that stream to the remote is delayed until accept is called.

QUIC has no notion of "acknowledgement of a stream", except insofar as that packets are acknowledged in general, but that happens unconditionally on receipt. This is part of why streams are so cheap to open; there's no round-trip involved. The intention is indeed that you drain accept eagerly.

However, then I am quite confused why accept exists in the first place instead of poll emitting a corresponding event.

Because QUIC streams are always opened in ID order, accept only needs to track the ID of the highest stream opened by the peer, and the highest one previously accepted. This is a lot more efficient than giving each stream a discrete entry in an event queue.

Documenting the meaning of accept would maybe be a good idea in any case, because the RFC doesn't use that kind of language around streams afaik.

I agree we could do better here. The RFC mostly doesn't concern itself with API details.

[FlorianUekermann]

Thanks for taking the time to clarify!

QUIC has no notion of "acknowledgement of a stream", except insofar as that packets are acknowledged in general, but that happens unconditionally on receipt.

To make sure I understand correctly: Calling accept has no effect on per stream flow control? So if I never call accept, that has the same effect as calling accept and never reading data from the stream?

The intention is indeed that you drain accept eagerly.

Ok. For me this advice boils down to:

match conn.poll() {
  quinn_proto::StreamEvent::Opened { dir } => while let Some(id) = conn.streams().accept(dir) {
    /// ... do my thing
  }
}

And then my problem is solved I guess. I'll have a think about what would have made the docs on accept more helpful to me.

If I may suggest a change that would add convenience: Include the maximum open stream id in that direction or the index in the event:

StreamEvent::Opened {
  dir: Dir,
  max_index: u64,
}

That would allow me to avoid surprises without draining accept eagerly.

---

I suspect the following comes down to preference:

This is a lot more efficient than giving each stream a discrete entry in an event queue.

With the code snippet above in mind, I'm not sure I follow the efficiency argument, because accept still gets called once per stream, like poll would if it emitted per stream open events. If the call to accept only increments the next_reported_remote counter and doesn't really feed back into quinn-proto at all, wouldn't these approaches be even more efficient:

• Do the next_remote[dir] != next_reported_remote[dir] check early in poll and emit per stream StreamEvent::Opened { id } events, without having to queue them up? How users deal with them (queue or counter) is up to them.
• State the range of opened StreamIds in StreamEvent::Opened and neither have per stream accept nor per stream StreamEvent::Opened.

In both cases accept could be removed, which imo makes the quinn-proto easier to understand.

[FlorianUekermann]

@Ralith : After sleeping on this once...

QUIC has no notion of "acknowledgement of a stream"

That is technically correct, but a peer still has to abide by the stream id limit communicated by MAX_STREAMS frames. This can be used to provide backpressure by not incrementing that limit until the app has "accepted" the previous stream in the respective direction. Could it be that this was the original intent behind introducing accept as opposed to just emitting the information via StreamEvent::Opened?

[Ralith]

I suspect the following comes down to preference

You're correct that poll could efficiently synthesize fine-grained stream opening events. I expect this would require most callers to implement logic similar to that of accept to track which streams have been yielded to application-layer logic and which remain queued. While that would technically make quinn-proto simpler, complicating most downstream code doesn't seem like a net win.

It may be instructive to refer to the higher-level quinn crate's use of this API. We do not drain incoming streams eagerly. Instead, we yield them one at a time when the application layer requests them via the high-level quinn::Connection::accept future. This happens concurrently with, and independent of, other connection events.

MAX_STREAMS [...] can be used to provide backpressure by not incrementing that limit until the app has "accepted" the previous stream in the respective direction.

Quinn only issues new stream ID credit when an existing stream is closed. This makes it easier to limit per-stream resource consumption., and make effective use of those resources.

Could it be that this was the original intent behind introducing accept

We did have similar behavior in a very old version, but I think the current semantics remain useful.

[FlorianUekermann]

Thanks a lot for your help. I think it is time to close.

You're correct that poll could efficiently synthesize fine-grained stream opening events. I expect this would require most callers to implement logic similar to that of accept to track which streams have been yielded to application-layer logic and which remain queued.

Whether accept stays or not doesn't matter much to me. I can see the use, as I'm indeed implementing equivalent logic after draining accept eagerly now (thanks a lot for your hints and explanation, that is a very viable solution for me). My original issue was that there were too many code paths where a stream id could appear for the first time, complicating the creation of bookkeeping items I can't do without atm. Without removing accept, stating the id ranges in StreamEvent::Opened would still be really great and wouldn't increase the size of the Event enum, because there are much large variants. But it is by no means necessary given a more accurate understanding of accept than before opening the issue. Just leaving these alternative proposals here:

StreamEvent::Opened {
  dir: Dir,
  max: StreamId,
}

use core::ops::Range;

StreamEvent::Opened {
  recv_idx: Range<u64>,
  bidi_idx: Range<u64>,
}

match stream_event {
  StreamEvent::Opened { recv_idx, bidi_idx } => {
    if !recv_idx.is_empty() { self.wake_opened_recv() }
    if !bidi_idx.is_empty() { self.wake_opened_bidi() }
  }
}

[Ralith]

Thanks for the feedback! We're always interested in opportunities to make the API friendlier.