Closed Bug 1081341 Opened 5 years ago Closed 5 years ago

h2 1xx support

Categories

(Core :: Networking: HTTP, defect)

32 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

(Whiteboard: [spdy])

Attachments

(1 file)

draft 14 made 1xx responses legal again, except for 101.

we missed that in the update. I've fixed it and managed to hand test it.

node-http2 also missed it, so I can't seem to write an automated case.. if you send the response header twice it (100 and 200) it gets all racy on you - and it only allows one response header per stream. we can add the automated case when node fixes the bug. (which I'll file).

The manual verification involved running a node-http2 case a bunch of times and examining the nspr logs to correlate the failures and passes with broken/working node behavior.
Whiteboard: [spdy]
Attached patch h2 1xx responsesSplinter Review
Attachment #8503442 - Flags: review?(hurley)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment on attachment 8503442 [details] [diff] [review]
h2 1xx responses

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

Stupid 1xx... needed to die.

I'll try to get to implementing it properly in node-http2 when I'm back from PTO on the 20th. (I actually think disallowing multiple responses in node-http2 was a fairly recent bugfix, so it should be easy enough to track down the proper place to change everything to allow multiple responses in this one case.)

::: netwerk/protocol/http/Http2Session.cpp
@@ +1214,5 @@
>    }
> +
> +  // if this turns out to be a 1xx response code we have to
> +  // undo the headers received bit that we are setting here.
> +  bool thisSetAllRecvd = !mInputFrameDataStream->AllHeadersReceived();

I'm not crazy about this name... it took me too much thinking to realize it meant "this action caused all received to be set when it wasn't previously". Maybe "didSetAllReceived"? But that seems ambiguous, too, since it's definitely calling SetAllHeadersReceived. Maybe "didFirstSetAllReceived"... ugh. I dunno.
Attachment #8503442 - Flags: review?(hurley) → review+
https://hg.mozilla.org/mozilla-central/rev/e1e2f5a56066
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.