SwiftQUIC: New inbound streams should enqueue the inboundDataAvailable event - #73
Conversation
|
Hm, this sounds more like a bug in the QUICTransfer code to me. The expectation is that for any new connected event, the upper protocol should try to read. It should only need to wait for an inbound data available event once it has tried to read and failed. This overall minimizes events, rather than enqueuing extra events that could be implicitly known. Can you fix the test instead? |
No, it’s not a bug in QUICTransfer. Today if you only get one chunk of inbound data you need to call read on the stream harness, but if you get a second at a later point then you can get a signal via inbound available. This seemed inconsistent and confusing and folks thought QUICTransfer was broken. The same could happen for someone adopting the protocol stack in the wild too. I can understand the performance argument here, as keeping the behavior this way will minimize stream events on the server. I think if we do keep this behavior as such then we need to document this in some way so that it does not confuse future folks wanting to use our APIs. |
Even though inconsistent, this does not add more events to the protocol stack and I can get onboard with that notion for performance reasons. Addressed 77b43ef |
| enum PendingEvent: ~Copyable { | ||
| case connected(_ from: ProtocolInstanceReference, _ to: ProtocolInstanceReference) | ||
| case disconnected(_ from: ProtocolInstanceReference, _ to: ProtocolInstanceReference, error: NetworkError?) | ||
| // inboundDataAvailable event will only be available on the second inbound datagram, the first will not trigger this event |
There was a problem hiding this comment.
Please remove this comment; it's not specifically accurate, and this layer doesn't know anything about datagrams, etc. This part is much more generic.
There was a problem hiding this comment.
I can remove it but we need to have this behavior documented somewhere so that it is known to either folks or LLMs researching this issue. Where do you suggest?
| @_spi(ProtocolProvider) | ||
| @available(Network 0.1.0, *) | ||
| public protocol InboundDataLinkage: UpperProtocolLinkage where PairedLinkage: OutboundDataLinkage { | ||
| // Will only notify a upper protocol for the second inbound datagram. |
There was a problem hiding this comment.
Please remove this comment; it's not specifically accurate, and this layer doesn't know anything about datagrams, etc. This part is much more generic.
There was a problem hiding this comment.
I can remove it but we need to have this behavior documented somewhere so that it is known to either folks or LLMs researching this issue. Where do you suggest?
tfpauly
left a comment
tfpauly
left a comment
There was a problem hiding this comment.
Technically the PR name should update, but content looks fine
2 participants
There has been a long-standing situation where a new inbound QUIC stream did not deliver a inbound data available event. You could work around this by doing an optimistic read and that is what is done on the server today. The idea is that if you received a new stream frame it was also likely to contain data.
This situation showed up though with the QUICTransfer benchmark. Folks that wanted to use it to measure 1 transfer could not do so because it would never finish the benchmark and just hang. This got me to dig in and fix this situation.
The side affect here will be more events on the server side so heads up @glbrntt.
I also took the time here to refactor the QUICTransfer benchmark to use a more refined write / read loop.
You should now be able to do: