quic: add support for HTTP/3 datagrams#64234
Conversation
Signed-off-by: Tim Perry <pimterry@gmail.com>
|
Review requested:
|
nodejs-github-bot
added
c++
Queue datagrams for pending streams
|
Now updated. This now buffer datagrams for pending streams that couldn't be immediately opened (thanks @martenrichter for the suggestion). Returns the id as normal when sending, reports statuses as normal, including ABANDONED if it wasn't possible to eventually unbuffer the datagrams. Queues up to highWatermark to match the normal write behaviour, then explicitly rejects future writes. We could consider drop-oldest instead, but that would mean there's no way to detect this behaviour at all since there's no backpressure mechanism here. Seems unlikely that stream opening will queue for long or that datagram traffic will get huge, so this seems reasonable for now but happy to debate. |
|
It is unfortunate that regular QUIC datagrams and HTTP/3 datagrams appear to have these different semantics. I'm not convinced that having two separate mechanisms const datagrams = session.datagrams; // Returns null if datagrams
// at the session-level are
// not supported
const datagrams = stream.datagrams; // Returns null if datagrams
// at the stream-level are
// not supported
// Same high-level API for both
datagrams.send(...);
datagrams.ondatagram = (...) => { /* ... */ };Further, in the QUIC docs, saying things like "Only applies to HTTP/3", etc is too specific. I would word it like, "Session-level datagrams may not be supported by all QUIC applications" and "Stream-level datagrams may not be supported by all QUIC applications", etc. |
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
See my comments here #64234 (comment)
| // On HTTP/3 sessions, raw unframed session-level datagrams are invalid | ||
| if (session->has_application() && | ||
| session->application().type() == Session::Application::Type::HTTP3) { | ||
| return args.GetReturnValue().Set(BigInt::New(env->isolate(), 0)); |
There was a problem hiding this comment.
We should keep application-specific handling into the Application subclass as much as possible.
|
|
||
| const kNilDatagramId = 0n; | ||
|
|
||
| const kApplicationTypeHttp3 = 2; |
There was a problem hiding this comment.
Nit: we should expose the type enum constants via the binding so we don't have to worry about keeping anything in sync.
| onStreamDatagram(uint8Array, early) { | ||
| debug('stream datagram callback', this[kOwner]); | ||
| this[kOwner][kDatagram](uint8Array, early); | ||
| }, |
There was a problem hiding this comment.
Do we really need to introduce a new callback? Alternatively, we could introduce a third argument to the existing callback that indicates that the datagram is associated with a specific stream.
|
|
||
| // Returns true if the application protocol supports a custom datagrams | ||
| // format, e.g. HTTP/3 datagrams bound to a request stream. | ||
| virtual bool SupportsDatagrams() const { return false; } |
There was a problem hiding this comment.
Nit: the naming is a bit confusing. An application may support datagrams but not need custom datagram handling. Perhaps something like SupportsCustomDatagrams or something similar?
| virtual bool SupportsDatagrams() const { return false; } | ||
|
|
||
| // Called when a QUIC DATAGRAM frame is received, so that custom formats | ||
| // can be handled by the Application. |
There was a problem hiding this comment.
Alternatively, we could just route all datagrams through the application ReceiveDatagram and just let the application handle custom formats transparently, without having to advertise via SupportsDatagrams
| bool started_ = false; | ||
| nghttp3_mem* allocator_; | ||
| Options options_; | ||
| const bool local_datagrams_enabled_; |
There was a problem hiding this comment.
Should this be in options_ instead?
| datagram_id id; | ||
| Store data; | ||
| }; | ||
| std::deque<PendingDatagram> pending_datagram_queue_; |
There was a problem hiding this comment.
Is this pending outbound or pending inbound? A comment to clarify here would be helpful.
| // handle. If the session is already gone there is nobody to notify. | ||
| if (!pending_datagram_queue_.empty()) { | ||
| if (!session_->is_destroyed()) { | ||
| for (auto& dgram : pending_datagram_queue_) { |
There was a problem hiding this comment.
Nit: just defensively, I'd copy the contents of pending_datagram_queue_ out to a temporary local. You then don't need to clear it below.
| // bound to a Quarter Stream ID. Buffer it and flush when the stream opens. | ||
| // If it cannot be buffered (queue full) the id is discarded. | ||
| if (stream->is_pending()) { | ||
| bool buffered = stream->EnqueuePendingDatagram(id, std::move(store)); |
There was a problem hiding this comment.
Instead of having EnqueuePendingDatagram return a boolean which then needs to be checked below, it could just return 0 when the datagram is not enqueued.
| stream->EndWriting(); | ||
| } | ||
|
|
||
| // Sends an HTTP/3 datagram (RFC 9297) associated with this stream. Returns |
There was a problem hiding this comment.
Let's remove the mentions of HTTP/3 here. It sends a datagram associated with this stream, only works if the application supports stream-level datagrams, etc.
| // Mint the id up front. It is exposed (returned non-zero to JS) only | ||
| // if the datagram is committed - queued now, or buffered for a pending | ||
| // stream. Sync rejection returns 0 and discards (no subsequent status). | ||
| datagram_id id = stream->session().ReserveDatagramId(); |
There was a problem hiding this comment.
There's a bit of a TOCTOU issue here. You're reserving the datagram id, but enqueuing it below may not accept it. We can end up burning datagram ids that are rejected.
| void DatagramReceived(const uint8_t* data, | ||
| size_t datalen, | ||
| DatagramReceivedFlags flag); | ||
| void DeliverRawDatagram(const uint8_t* data, |
There was a problem hiding this comment.
A comment here explaining the difference between DatagramReceived and DeliverRawDatagram would be helpful
| Store&& payload, | ||
| datagram_id id) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Maybe we should have the Session level SendDatagram (for both session and stream level) defer to the Application and make the Stream* argument optional. I can foresee future QUIC applications applying their own semantics to datagrams even at the session level, so adjusting for it now makes sense.
5 participants
With nghttp3 released and updated here, we can now cleanly add proper HTTP/3 datagram support to fix #63891. This PR:
stream.sendDatagramandstream.ondatagramsupport on HTTP/3 streams.The resulting API is a little awkward (both H3 & QUIC methods present everywhere, but throwing in different cases) due to the mixed QUIC/H3 API as already discussed, but I think it's worthwhile opening in the meantime anyway so people can start playing with this, and so we can review it standalone.
The API will conflict with the HTTP/3 split PR that restructures related areas, but the internals should be the same in any case (just moved around) so it'll be easy to slide into whatever final structure we end up with there.
This doesn't support HTTP/3 datagrams for 0RTT on the client side. The RFC would require us to persist the HTTP/3 SETTINGS client-side to validate datagrams are supported to do so, but we currently don't persist HTTP/3 SETTINGS at all. Right now for all HTTP/3 0RTT we're just falling back to the HTTP/3 defaults instead (as allowed by the spec) but the datagram default is off. We could explore that later if we want, but completely rejigging HTTP/3 session resumption to enable this seemed more effort than it's worth for now. Server acceptance of inbound 0RTT datagrams from non-Node clients should work fine but can't be covered in the tests due to this.