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.
Happy to cover the other comments, but quite a few here are clearly focused on providing a single QUIC+HTTP datagram API. I really feel the whole HTTP/3 API doesn't currently work well without some kind of explicit API split between QUIC & HTTP/3. This PR is designed to easily refactor into that split model later once we have it, so I don't really want to redesign it to fit a single combined API in the meantime. We need to talk about this and resolve it, here or in #63995 or directly, it's blocking a few things now. I understand you prefer to have a single pair of classes (Session/Stream) act as the API for both separate protocols at the same time, but I'm not clear why, and there isn't any more context or explanation behind that which I can see anywhere. Can you explain when & why that API is preferable to the separate QuicSession/Stream + Http3Session/Stream alternative? For reference, I think my position is captured well in the 'Background' of the PR description for #63995. |
I'm focused on providing a QUIC API with affordances to implement a unified http api around it. We already have a mess with having separate |
This is what the current API already does though. Today we have an HTTP/3 specific API, it's just smooshed in on top of the QUIC API. All the methods and events and fields are there and exposed right now. It's actively inconsistent with the other HTTP APIs, and separate from the existing internals (e.g. totally separate TLS config), and the API is entirely unique & specific for HTTP/3. We're not currently avoiding this problem. I want to reduce that gap and make this more maintainable: aligning more closely, and making the split explicit via QUIC + HTTP/3 classes you compose (just like net.Server/tls.Server/http.Server) rather than one implementation with implicit combined behaviour. I'm not suggesting we change from the API features exposed today, or change the internals beyond moving things around. I do understand you'd rather not have an actual If you really want to go this direction though, we should drop HTTP/3 here entirely, and go QUIC-only. If we have to choose, I'd prefer this - we could then implement HTTP/3 primitives in userspace at least, which is impossible with the current API. There are arguments in this direction, e.g. QUIC-only is the current state of Deno (https://docs.deno.com/examples/quic/). Would you prefer that? Personally, I'd like have a proper API for HTTP/3 in Node itself, but doing pure QUIC for now would be better than mixing them all together in a way they can't be used independently like today. We could drop HTTP/3 and potentially explore as a separate topic later on.
I'd love this, fully agreed, happy to work on it directly and I've had other discussions about it. But they aren't mutually exclusive. The opposite: to build a unified HTTP API, we need a good HTTP/3 API somewhere (at least internally) to build it on. The current API would be difficult even for the universal API internal use case, since it diverges so much from the HTTP/1 & HTTP/2 & TLS modules. Personally though, I have some limited need for a unified API, and a very strong need every day for direct control over HTTP/3 and QUIC. Most simple servers & clients would appreciate a unified version, but serious networking libraries/tools/infra need to touch specific protocol primitives. The differences do matter. There are clearly large numbers of people doing low-level networking (lower than 'generic HTTP') on top of Node who are interested in specific protocol support (see the interest on #38478). Given that we must implement the primitives anyway, I think we should expose them in a way people can use them. More philosophically: I think it's more important for Node to provide the core primitives than unifying broader features (ofc, in a perfect world we do both). Userspace can implement the latter - it's much much harder to externally implement the former. Is there a route through here? Happy to explore the specifics of how we do this in #63995, I'm very flexible on the details within this. I haven't updated that from other discussion or rebased on the other PRs but I can this week if you'd be interested. Or would you prefer to drop HTTP/3 from QUIC entirely for now? That would solve the concern in practice and work for me. Or are we stuck, and this is a topic for a TSC debate? |
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.