Closed
Bug 1001022
Opened 10 years ago
Closed 10 years ago
http/2 draft 12
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: u408661, Assigned: u408661)
References
Details
(Whiteboard: [spdy])
Attachments
(2 files)
28.11 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
52.06 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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! :)
This builds on top of the draft11 patches in bug 993037. Pretty straightforward, not a large delta on this draft.
Attachment #8412171 -
Flags: review?(mcmanus)
And here's the one for the tests. Pull requests are on github for the node-http2 changes.
Attachment #8412172 -
Flags: review?(mcmanus)
(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•10 years ago
|
||
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.
Attachment #8412171 -
Flags: review?(mcmanus) → review+
Updated•10 years ago
|
Attachment #8412172 -
Flags: review?(mcmanus) → review+
(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.
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3125aa54ca3e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e2228862e8
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3125aa54ca3e https://hg.mozilla.org/mozilla-central/rev/b5e2228862e8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 9•10 years ago
|
||
(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?
You need to log in
before you can comment on or make changes to this bug.
Description
•