1001022 - http/2 draft 12

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

We may not have landed draft11 yet, but draft12 is out, and I'm ready to implement!

http://tools.ietf.org/html/draft-ietf-httpbis-http2-12
HPACK does not appear to have a new draft associated with it

Comment 1

[Patrick McManus [:mcmanus]]

sorry I'm jammed up.. I keep thinking I'm almost ready to get to my backlog and I keep finding more problems with the code I'm working on.

otoh - careful what you wish for, cause you're going to review it! :)

Comment 2

[u408661]

Attachment: part 1 - client implementation

This builds on top of the draft11 patches in bug 993037. Pretty straightforward, not a large delta on this draft.

Comment 3

[u408661]

Attachment: part 2 - tests

And here's the one for the tests. Pull requests are on github for the node-http2 changes.

Comment 4

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #1)
> sorry I'm jammed up.. I keep thinking I'm almost ready to get to my backlog
> and I keep finding more problems with the code I'm working on.

Isn't that always how it goes?

> otoh - careful what you wish for, cause you're going to review it! :)

Worse things have happened (I think).

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8412171 [details] [diff] [review]
part 1 - client implementation

Review of attachment 8412171 [details] [diff] [review]:
-----------------------------------------------------------------

jpinner tweeted twitter is live with h2-12. interop?

r+ assuming you're ok with removing blocked.. re r? if you would rather change it.

::: netwerk/protocol/http/Http2Session.h
@@ +121,5 @@
>      SETTINGS_TYPE_HEADER_TABLE_SIZE = 1, // compression table size
>      SETTINGS_TYPE_ENABLE_PUSH = 2,     // can be used to disable push
>      SETTINGS_TYPE_MAX_CONCURRENT = 3,  // streams recvr allowed to initiate
> +    SETTINGS_TYPE_INITIAL_WINDOW = 4,  // bytes for flow control default
> +    SETTINGS_TYPE_COMPRESS_DATA = 5 // whether other side allowes compressed DATA

let's add a COMPRESS_DATA = 0 to our SendHello() settings frame so that we're real nice and explicit about that.

::: netwerk/protocol/http/Http2Stream.cpp
@@ +455,5 @@
>        (idx) ? Http2Session::FRAME_TYPE_CONTINUATION : Http2Session::FRAME_TYPE_HEADERS,
>        flags, mStreamID);
>      outputOffset += 8;
>  
>      if (!idx) {

add comment dependency 0, weight is priority weight, not exclusive

@@ +950,3 @@
>  {
> +  // XXX - we ignore dependency and exclusive for now
> +  mPriorityWeight = newWeight;

basically this doesn't matter, because really why is the server sending priority frames?

nonetheless, just taking the weight isn't really right.. so let's at least LOG it :)

@@ +1049,5 @@
>        LOG3(("Http2Stream this=%p, id 0x%X request body suspended because "
>              "remote window is stream=%ld session=%ld.\n", this, mStreamID,
>              mServerReceiveWindow, mSession->ServerSessionWindow()));
> +      if (!mBlockedOnRwin) {
> +        TransmitBlockedFrame();

I'm not sure that this code adheres to this:
"an endpoint MUST NOT send any subsequent BLOCKED frames until the
   affected flow control window becomes positive."

and i'm pretty sure google.com will protocol error us of it believing its a dos.

rather than fixing it, my preference would be just to not generate it. up to you though.

::: netwerk/protocol/http/nsHttp.h
@@ +33,5 @@
>          // 24 was a internal spdy/3.1
>          // 25 was spdy/4a2
>          // 26 was http/2-draft08 and http/2-draft07 (they were the same)
> +        // 27 was also http/2-draft09, h2-10, and h2-11
> +        HTTP2_VERSION_DRAFT12 = 27

since -12 is going to be the big test run, let's mint a new ID to get clean telemetry.

Comment 6

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8412171 [details] [diff] [review]
> part 1 - client implementation
> 
> Review of attachment 8412171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> jpinner tweeted twitter is live with h2-12. interop?

https://twitter.com/todesschaf/status/461314423559176192 :)

> r+ assuming you're ok with removing blocked.. re r? if you would rather
> change it.
> 
> ::: netwerk/protocol/http/Http2Session.h
> @@ +121,5 @@
> >      SETTINGS_TYPE_HEADER_TABLE_SIZE = 1, // compression table size
> >      SETTINGS_TYPE_ENABLE_PUSH = 2,     // can be used to disable push
> >      SETTINGS_TYPE_MAX_CONCURRENT = 3,  // streams recvr allowed to initiate
> > +    SETTINGS_TYPE_INITIAL_WINDOW = 4,  // bytes for flow control default
> > +    SETTINGS_TYPE_COMPRESS_DATA = 5 // whether other side allowes compressed DATA
> 
> let's add a COMPRESS_DATA = 0 to our SendHello() settings frame so that
> we're real nice and explicit about that.

Good plan; done.

> ::: netwerk/protocol/http/Http2Stream.cpp
> @@ +455,5 @@
> >        (idx) ? Http2Session::FRAME_TYPE_CONTINUATION : Http2Session::FRAME_TYPE_HEADERS,
> >        flags, mStreamID);
> >      outputOffset += 8;
> >  
> >      if (!idx) {
> 
> add comment dependency 0, weight is priority weight, not exclusive

Done.

> @@ +950,3 @@
> >  {
> > +  // XXX - we ignore dependency and exclusive for now
> > +  mPriorityWeight = newWeight;
> 
> basically this doesn't matter, because really why is the server sending
> priority frames?
> 
> nonetheless, just taking the weight isn't really right.. so let's at least
> LOG it :)

OK, LOG it is!

> @@ +1049,5 @@
> >        LOG3(("Http2Stream this=%p, id 0x%X request body suspended because "
> >              "remote window is stream=%ld session=%ld.\n", this, mStreamID,
> >              mServerReceiveWindow, mSession->ServerSessionWindow()));
> > +      if (!mBlockedOnRwin) {
> > +        TransmitBlockedFrame();
> 
> I'm not sure that this code adheres to this:
> "an endpoint MUST NOT send any subsequent BLOCKED frames until the
>    affected flow control window becomes positive."
> 
> and i'm pretty sure google.com will protocol error us of it believing its a
> dos.
> 
> rather than fixing it, my preference would be just to not generate it. up to
> you though.

I'm absolutely fine with this. I went back and forth on whether or not to include it at all, and came to a very narrow decision to include it (mostly because it was easy to do the way it is now - though I definitely wasn't sure it 100% adhered to the spec, I figured you'd know enough to say so).

> ::: netwerk/protocol/http/nsHttp.h
> @@ +33,5 @@
> >          // 24 was a internal spdy/3.1
> >          // 25 was spdy/4a2
> >          // 26 was http/2-draft08 and http/2-draft07 (they were the same)
> > +        // 27 was also http/2-draft09, h2-10, and h2-11
> > +        HTTP2_VERSION_DRAFT12 = 27
> 
> since -12 is going to be the big test run, let's mint a new ID to get clean
> telemetry.

done.

Comment 7

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3125aa54ca3e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e2228862e8

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3125aa54ca3e
https://hg.mozilla.org/mozilla-central/rev/b5e2228862e8

Comment 9

[Dave Garrett]

(In reply to Nicholas Hurley [:hurley] from comment #6)
> (In reply to Patrick McManus [:mcmanus] from comment #5)
> > jpinner tweeted twitter is live with h2-12. interop?
> 
> https://twitter.com/todesschaf/status/461314423559176192 :)

If sites are going to implement this version, would it be viable to uplift draft 12 to Aurora to get HTTP2 testing out to a wider audience?