Map HTTP/2 Streams to Channels; branch 4.1#4503
Conversation
|
@Scottmitch, PTAL. Not many changes, and I hope I've made it easy to review after the rebase. As I mention, the base commit now creates a new example, but I hope that is easy to review since I only changed import/package and run-example.sh. @nmittler, FYI. |
There was a problem hiding this comment.
do we want to use some other default size ?
There was a problem hiding this comment.
How about defining a new SocketAddress type whose member fields are:
- the actual
SocketAddressof the underlying socket connection - the stream ID
There was a problem hiding this comment.
For the moment I'm purposefully not exposing the stream id. Applications don't care about the stream id, and using an object reduces the need for each handler to keep its own int -> stream object mapping. On the child channel the "stream object" is the channel itself, so there's not a gain in exposing any more information here at present.
As we implement more of the features if we end up needing to expose more information, I'm game. But until we have a need, I'd rather leave it limited.
|
Left a few trivial comments. I see some comments from SonarQube or @normanmaurer were not addressed yet though. |
|
@normanmaurer @trustin, comments addressed or replied to, PTAL @trustin, the only SonarQube comments I see are for the example, where they seem dubious and are prexisting in the current example. If I make fixes to the new copy I'd really want to fix the existing one as well, but I'd rather not do that in this PR to avoid scope creep. |
There was a problem hiding this comment.
Complete the task associated to this TODO comment. 
|
@normanmaurer @trustin is there anything else that needs to be done here? anything I can do to help get this merged? we're excited to start playing with this after it gets into netty. |
|
@mosesn - It seems everyone is content with this. I will pull in tomorrow if no objections. |
|
+1
|
|
Awesome, super excited! |
|
@ejona86 please squash and I will cherry-pick :) Thanks again |
Motivation: This allows using handlers for Streams in normal Netty-style. Frames are read/written to the channel as messages, not directly as a callback/method call. Handlers allow mixing and can ease HTTP/1 and HTTP/2 interoperability by eventually supporting HTTP/1 handlers in HTTP/2 and vise versa. Modifications: New handler Http2MultiplexCodec that converts from the current HTTP/2 API to a message-based API and child channels for streams. Result: The basics are done for server-side: new streams trigger creation of new channels in much the same appearance to how new connections trigger new channel creation. The basic frames HEADERS and DATA are handled, but also GOAWAY and RST_STREAM. Inbound flow control is implemented, but outbound is not. That will be done later, along with not completing write promises on the child channel until the write actually completes on the parent. There is not yet support for outbound priority/weight, push promises, and many other features. There is a generic Object that may be set on stream frames. This also paves the way for client-side support which needs a way to refer to yet-to-be-created streams (due to how HEADERS allocates a stream id, and the allocation order must be the same as transmission order).
ejona86
force-pushed
the
http2-stream-as-channel-4.1
branch
from
48bf44b to
a6e5cb1
Compare
|
@normanmaurer, squashed |
|
Thanks everyone! This has been a long ride with about a year of review and over 500 comments! Now we just need to do the rest of the things from #3667. |
|
@Scottmitch @ejona86 fwiw, I'd love to help out if I can. I'm hoping to build finagle's http2 server implementation against your new API. |
7 participants
This allows using handlers for Streams in normal Netty-style. Tracking issue for larger set of changes along this vein is #3667.
This is a continuation of #3666, but against 4.1 instead of master. The "Map HTTP/2 Streams to Channels" commit is approximately where #3666 left off. The only difference is that the new example was created instead of modifying the pre-existing sample.
Fixing things to actually compile after rebasing is a separate commit to make it obvious what changes/hacks were needed. Overall, very few changes were made since the last review, but I'm now not aware of any blocking issues for to to be merged so that we can begin incremental improvements. Based on the last review, it seems we should be really close now.