quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

`Readable` event not queued on first stream open when data is readable. #1281

Closed TimonPost closed 2 years ago

TimonPost commented 2 years ago

Given the following events:

Readable is generated in StreamState::on_stream_frame

    /// Notify the application that new streams were opened or a stream became readable.
    fn on_stream_frame(&mut self, notify_readable: bool, stream: StreamId) {
        if stream.initiator() == self.side {
            // Notifying about the opening of locally-initiated streams would be redundant.
            if notify_readable {
                self.events.push_back(StreamEvent::Readable { id: stream });
            }
            return;
        }
        let next = &mut self.next_remote[stream.dir() as usize];

        if stream.index() >= *next {
            *next = stream.index() + 1;
            self.opened[stream.dir() as usize] = true;
        } else if notify_readable {
            self.events.push_back(StreamEvent::Readable { id: stream });
        }
    }

The flag for opened streams is also set here. This means that when we process an incoming frame it either marks an opening stream or a readable event. While, as a user, I would expect both an Opened and Readable event when I receive data 'and' a stream is opened in the same packet.

Why should we care When creating an FFI, or implementation of quinn-proto it can be confusing as one assumes to receive a Readable event when data is Readable. But instead, data could be ready to be read on Opened event as well.

Solutions:

TimonPost commented 2 years ago

Not related to this issue, but a question I had regarding a note in this code in on_stream_frame:

// Notifying about the opening of locally-initiated streams would be redundant.

However, if stream.initiator() == self.side checks if initiated stream is created on the local endpoint side. So why, if this is redundant, is this code still queuing the Readable event? I am probably misinterpreting this comment or code so that's why I ask what I am missing?

Ralith commented 2 years ago

I agree that the documentation here is unclear. We should not generate Readable events for every Opened event, because that adds no extra information. We could instead generate Readable events more precisely (i.e. guarantee that a read operation will not be blocked), but it's unclear to me if this is worth the effort. Hence I think the best solution is to document that Opened implies Readable, and replace "has data or errors" with "may have data or errors" to communicate that false positives are permitted.

why, if this is redundant, is this code still queuing the Readable event

Because the Readable event notifies about a stream becoming readable. As the comment indicates, only notifying about the opening of a locally-initiated stream would be redundant. An application inherently knows whether it has opened a locally initiated stream or not, but relies on events to determine when such a stream is readable.

Ralith commented 2 years ago

I think this was fixed in #1284.