http3: support WebTransport #3256
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3256 +/- ##
==========================================
- Coverage 85.69% 83.62% -2.07%
==========================================
Files 132 140 +8
Lines 9775 10197 +422
==========================================
+ Hits 8376 8527 +151
- Misses 1024 1279 +255
- Partials 375 391 +16
Continue to review full report at Codecov.
|
|
@marten-seemann could I get your feedback on this PR? thanks! |
| @@ -89,7 +89,7 @@ func (s *closedLocalSession) destroy(error) { | |||
| }) | |||
| } | |||
|
|
|||
| func (s *closedLocalSession) getPerspective() protocol.Perspective { | |||
| func (s *closedLocalSession) Perspective() Perspective { | |||
There was a problem hiding this comment.
I still think this should be handled on the http3 layer, not on the quic layer.
| writeDatagram(SessionID, []byte) error | ||
| } | ||
|
|
||
| type connection struct { |
There was a problem hiding this comment.
To elaborate, you can have a bool here which tells you if this is a client or a server connection. Then you don't need to ask the Session for its Perspective(). You just have to set that bool once when you initialize this struct.
There was a problem hiding this comment.
How would Open or Accept know to error on the wrong perspective if it's not exposed at the quic layer?
| streamType := StreamType(t) | ||
|
|
||
| // Store control, QPACK, and push streams on conn | ||
| if streamType < 4 { |
There was a problem hiding this comment.
Where does this magic number come from?
There was a problem hiding this comment.
4 standard H3 streams: control, qpack, and push (0x0-0x3).
http3/conn.go
Outdated
| incomingStreamsMutex sync.Mutex | ||
| incomingStreams map[SessionID]chan quic.Stream // Lazily constructed | ||
|
|
||
| incomingUniStreamsOnce sync.Once |
There was a problem hiding this comment.
Would it make sense to start the go routine right away, and get rid of this sync.Once? Every HTTP/3 session will use unidirectional streams.
There was a problem hiding this comment.
True.
Deferring it simplifies the test mocking. My thought was that a Conn wouldn't start any goroutines until asked to minimize resource consumption.
Thanks! I'll throw an example together. Are you OK with an examples directory, or would you prefer a separate repo? FWIW I'm not happy yet with the WebTransport API here. It's still a bit clunky for the server side. |
Since WebTransport will be explicitly supported by the http3 package, it’s justified (and super helpful!) to have an example in this repository. Thank you for all the effort you’re putting into this! |
|
@marten-seemann I merged my other WIP branch into this one, which reorganizes the HTTP/3 primitives into Next up is building a WebTransport example and tests. I’d love it if you could take another look, thanks! |
|
@marten-seemann any reason you’re still keeping CircleCI around? Doesn’t GH Actions cover these? |
|
Landed integration tests for WebTransport! |
|
Demo server built on this code: https://basic.webtransport.dev/ |
|
Multiplayer mouse cursor demo: https://cursors.webtransport.dev/ |
There was a problem hiding this comment.
Hi @ydnar,
I'm sorry for the late review. I really appreciate all the effort you're putting into is! The reason I'm so slow with reviews here is that I honestly don't know how to review a PR that touch 5k LOC. Going through it commit-by-commit is also not feasible, considering that this branch contains >200 commits.
It would be a lot easier and faster to review if these were separate, independent PRs.
From a quick glance over your PR, I'm not sure if all changes here are actually used and needed to make WebTransport work. For example, it's not clear to me
- what the CAPSULE frame does. You're defining it, but it doesn't seem to be used (unless I'm missing something)
- why we're adding HTTP Trailer support in this PR. As far as I understand, it's not needed for WebTransport, is it?
If they're actually not necessary, could you remove them from this PR (along with all the other things that are not needed)? If they are necessary, they sound like changes that could be split into separate PRs.
| onFrameError func() | ||
|
|
||
| bytesRemainingInFrame uint64 | ||
| onTrailers trailerFunc |
There was a problem hiding this comment.
Is trailer parsing something we have to do in this PR? It would be really helpful to separate stuff like this into smaller, self-contained PRs.
There was a problem hiding this comment.
Trailers are not required for WebTransport, but were trivial to add using the new http3.Conn and RequestStream primitives. I can separate this out.
| // Settings specifies the HTTP/3 settings transmitted to the server. | ||
| // If nil, reasonable default values will be used. | ||
| // See https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-http-2-settings-parameters. | ||
| // If non-nil, overrides EnableDatagrams and EnableWebTransport. |
There was a problem hiding this comment.
Why do we need this? I don't like the fact that this overrides other options.
Do we need to export Settings at all? Ideally, the user of the library wouldn't have to deal with setting code points at all.
There was a problem hiding this comment.
My preference is to remove EnableDatagrams and EnableWebTransport in favor of letting the caller specify Settings.
The Settings type has helpers for enabling datagrams or WebTransport, or other common settings.
| ) | ||
|
|
||
| // StreamType represents an HTTP/3 stream type. | ||
| type StreamType uint64 |
There was a problem hiding this comment.
Why is this needed? Why is it exported?
|
|
||
| // A ContextExtensionType represents an HTTP datagram context extension type code. | ||
| // https://www.ietf.org/archive/id/draft-ietf-masque-h3-datagram-03.html#name-context-extension-types | ||
| type ContextExtensionType uint64 |
There was a problem hiding this comment.
This type seems entirely unused.
There was a problem hiding this comment.
CapsuleType and ContextExtensionType are intended to support the HTTP/3 MASQUE datagram draft, which the WebTransport draft is converging against.
This can be deferred to a future PR.
| // value will be clamped to GreaseMax. | ||
| func writeGreaseFrame(w quicvarint.Writer, n uint64) { | ||
| quicvarint.Write(w, Grease(n)) | ||
| quicvarint.Write(w, 0) // Zero frame payload length |
There was a problem hiding this comment.
Better: this should probably write a (short) frame.
|
|
||
| // RequestHeaders returns valid HTTP/3 header fields for req, or an error if req | ||
| // is malformed. | ||
| func RequestHeaders(req *http.Request) ([]qpack.HeaderField, error) { |
There was a problem hiding this comment.
Not sure where this is coming from. I assume that you renamed a file and now Git recognizes it as new? It would be helpful if you revert the renaming, so Git can show the actual changes that happened here.
There was a problem hiding this comment.
It wasn’t so much of a rename as a refactor of (*requestWriter).writeHeaders. The rest of requestWriter was subsumed by http3.client and http3.RequestStream.
The requestWriter type is now gone.
| return id&0b11 == 0 | ||
| } | ||
|
|
||
| type requestStream struct { |
There was a problem hiding this comment.
Where does this come from? I assume some renamed file?
There was a problem hiding this comment.
The requestStream type is entirely new.
|
|
||
| // A CapsuleType represents an HTTP capsule type. | ||
| // https://www.ietf.org/archive/id/draft-ietf-masque-h3-datagram-03.html#name-capsule-types | ||
| type CapsuleType uint64 |
There was a problem hiding this comment.
This doesn't seem to be used anywhere, is it?
There was a problem hiding this comment.
CapsuleType and ContextExtensionType are intended to support the HTTP/3 MASQUE datagram draft, which the WebTransport draft is converging against.
This can be deferred to a future PR.
| // If a frame has already been partially read from R, a FrameReader | ||
| // can be initialized by setting Type to the current frame type and N to | ||
| // the number of bytes remaining in the payload. | ||
| type FrameReader struct { |
There was a problem hiding this comment.
Why is this exported? What was wrong with the parseNextFrame function?
There was a problem hiding this comment.
FrameReader allows the caller to determine which frames to skip, and makes no attempt to parse the underlying frames. This was necessary because different stream types (and perspectives) support specific frame types, and the spec(s) require closing the stream or connection in the event of certain frame types.
There was a problem hiding this comment.
It’s exported so callers implementing lower-level parts of the HTTP/3 protocol could use the same mechanism. In theory, this could go in a http3guts package.
|
|
||
| // A FrameType represents an HTTP/3 frame type. | ||
| // https://www.ietf.org/archive/id/draft-ietf-quic-http-34.html#name-frame-definitions | ||
| type FrameType uint64 |
There was a problem hiding this comment.
FrameType constants are (IETF) spec-defined values which can be used by callers implementing parts of HTTP/3 directly.
Will rename this back later.
Use Nanoseconds >> 10.
Replace SessionID type with uint64
… trailers This is a squashed commit of quic-go#3256. Subsequent commits will remove: - WebTransport - Trailers - Preemptive MASQUE/DATAGRAM features To slim down this PR.
Hey @ydnar |
|
@ydnar Are you going to participate in the WebTransport interop testing? https://mailarchive.ietf.org/arch/msg/webtransport/8c2dp_MP9GQEcnmurndjwAx8sOI/ |
I'd love to. I'll need your help though. Have you had time to look at #3284? |
|
Not yet. Is that the replacement for this PR? |
|
#3284 is a discrete PR that implements the foundation necessary to support the datagram draft, capsule support, WebTransport, and HTTP trailers. I broke it out 3 weeks ago at your request. See the #quic-go channel in Slack for more context. The commits can be reviewed in reverse order to see how each of the above features depend on it. If you can review and merge that, we can start adding WT or the datagram draft next. |
Now I'm really confused. Why first add features, and then remove them in subsequent commits? I have no idea how to start reviewing this. |
This PR is big. At your request, I broke it down into discrete parts.
|
… trailers This is a squashed commit of quic-go#3256. Subsequent commits will remove: - WebTransport - Trailers - Preemptive MASQUE/DATAGRAM features To slim down this PR.
|
@marten-seemann I updated the description on #3284 to clarify what it is. How can I help you get #3284 reviewed and merged so we can land datagram and WebTransport support? |
|
Wow this looks great can’t wait to try this compared to web sockets |
|
is there a possibility that this will be merged ? Are there technical blockers that your worried about or is it more just lack of time ? |
|
@ydnar - I am very interested in this. I think that WebTransport support is important for the future of quic-go, and it's sad to see it has not yet been merged. A couple of questions. @marten-seemann / @lucas-clemente can you share whether this not being merged is due to lack of time or lack of interest? You sharing that with us would be very helpful to the community on how best to move forward. If it is lack of interest, we respect that as it is your project after all, but communicating that directly to us would be helpful to prevent further development work on a PR that will never get merged. So, I would appreciate very much if you could take a few moments to do that. Thank you! @ydnar - can you kindly share the disco/DISCO bidirectional stream example server (in other words, what's running at 'the other end' of your browser screenshot) to make this easier to test? Thank you. |
Sure. I need to dust off the branch that enables this. I’d prefer not to maintain a fork of quic-go just to support WebTransport (or a fork of quic/http3). There’s some relevant stuff in Go 1.18 that should improve QUIC performance too (allocation-free UDP, net/netip package). |
|
Hi @ydnar - actually, I got it figured out. I decided to develop a new Go package which implements a WebTransport server over HTTP/3 (updated per the latest IETF drafts). Its API is inspired by the PR’s API but it implements it with the standard quic-go package as a dependency. It has pretty complete support for all of WebTransport including datagrams, server and client initiated uni/bidi streams and so on. It’s currently fully functional but needs docs written and further testing. I’ll share the link when ready if interested. |
Sure, I’ll take a look! |
@ydnar - great! Package is here: github.com/adriancable/webtransport-go Example server & client are here: github.com/adriancable/webtransport-go-example GoDoc is here: pkg.go.dev/github.com/adriancable/webtransport-go Hope that someone finds this useful. |
|
Closing, since there's now a WebTransport implementation using quic-go: https://github.com/marten-seemann/webtransport-go. |
This PR adds initial support for the WebTransport draft.
To see this in action, open these demos in Chrome Canary:
Background
Internally, it exposes a bit of additional API from the
http3package, including anhttp3.Connwhich wrapsquic.EarlySessionand anhttp3.RequestStreamwhich wrapsquic.Stream.An
http3.Connallows client and server code to share common stream and frame handling, as well as providing a foundation for (de)multiplexing datagrams in support of the MASQUE datagram draft. Anhttp3.RequestStreamis used to read and writeHEADERSandDATAframes, as well as process other HTTP/3 frames like MASQUECAPSULEframes.http3.ServerConnis constructed usinghttp3.Accept(quic.EarlySession, Settings).http3.ClientConnis constructed usinghttp3.Open(quic.EarlySession, Settings).http3.RequestStreamis accepted from(ServerConn).AcceptRequestStream(context.Context).http3.RequestStreamis opened with(ClientConn).OpenRequestStream(context.Context).It is possible to construct alternate HTTP/3 clients or servers using these primitives.
This PR builds on #3238 and #3254. It fixes #3191, and replaces #3226. It also adds support for HTTP trailers, modeled after the trailers interface in the standard
net/httppackage.To do
http3.RoundTripperScreenshots
Chrome demonstrating WebTransport unidirectional, bidirectional streams, and datagrams: