use h2 per stream flow control with suspended channels

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

32 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [spdy])

Attachments

(4 attachments)

h2 consumers are implemented a lot like h1 consumers - they can block the transport in the event that the data sink stops consuming data.

in general that doesn't matter - but h2 can do better.. channels can suspend() themselves (paused downloads!) and the streams api that is part of the fetch api will have the notion of suspending as well (xhr can't do that).

So this patch allows transactions that are running into suspended channels (the channel pipe is full), to buffer more data from the session so that other streams can proceed. While this is happening it does not ack (window_update) those buffered bytes which will cause the sender to eventually stop sending on that stream.

the patch also sets a lower default pull window (down from 256MB)- still very large to account for big BDP, because this window is the amount of data that will need to be buffered and it also allows channels to override that setting on a per channel basis.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
there are some jetpack perma fails from that run in comment 3, but they also show up in a different run (linked below) rooted in the same changeset as that patch - so we can feel ok not attributing them to this patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d82c63acee7b
Comment on attachment 8660914 [details] [diff] [review]
use h2 per stream flow control to deal with suspended channels

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

Looks pretty decent to me. Hooray flow control!

::: netwerk/protocol/http/Http2Stream.cpp
@@ +269,5 @@
> +
> +nsresult
> +Http2Stream::BufferInput(uint32_t count, uint32_t *countWritten)
> +{
> +  char buf[32768];

Let's make this a constant somewhere.

@@ +702,5 @@
> +  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
> +  if (trans && trans->InitialRwin()) {
> +    bump = (trans->InitialRwin() > mClientReceiveWindow) ?
> +      (trans->InitialRwin() - mClientReceiveWindow) : 0;
> +    } else {

nit: line indented one level too far (tab/space mix?)
Attachment #8660914 - Flags: review?(hurley) → review+
Comment on attachment 8660909 [details] [diff] [review]
test for h2 per stream flow control

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

::: netwerk/test/unit/test_http2.js
@@ +285,5 @@
>  
>    return chan;
>  }
>  
> +var SimpleListener = function() {};

Call this something other than SimpleListener (it has no obvious relation to its purpose). Perhaps "ResumeStalledChannelListener" or something.

@@ +323,5 @@
> +// test a large download that creates stream flow control and
> +// confirm we can do another independent stream while the download
> +// stream is stuck
> +function test_http2_blocking_download() {
> +  dump("starting test_http2_blocking_download\n");

Get rid of the dump.
Attachment #8660909 - Flags: review?(hurley) → review+
> 
> Call this something other than SimpleListener (it has no obvious relation to
> its purpose). 

Ha! that's because you didn't see the wip version it replaced :) (fixed)


> > +  dump("starting test_http2_blocking_download\n");
> 
> Get rid of the dump.

no way :) I like me some minimal orientation output - that test when it fails pretty much just hangs there, so its good to know what state we are in. Its just one line of output.
(In reply to Patrick McManus [:mcmanus] from comment #7)
> > > +  dump("starting test_http2_blocking_download\n");
> > 
> > Get rid of the dump.
> 
> no way :) I like me some minimal orientation output - that test when it
> fails pretty much just hangs there, so its good to know what state we are
> in. Its just one line of output.

Alright, then let's at least add similar dumps to the rest of the test_foo functions, so it's not just one random dump hanging out there :)
https://hg.mozilla.org/mozilla-central/rev/0bf012ea842f
https://hg.mozilla.org/mozilla-central/rev/dfd4e61be062
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1208114
Depends on: 1211706
Comment on attachment 8671554 [details] [diff] [review]
backout dfd4e61be062 on aurora (gecko 43)

this feature is on 43 and 44, but it has a couple of dependent fixes currently in the process of landing/reviewing on 44. Rather than backporting them I'd like to back this out of 43 and let it bake another round on 44.

Approval Request Comment
[Feature/regressing bug #]: 1204614
[User impact if declined]: we would have to backport dependent fixes
[Describe test coverage new/current, TreeHerder]: backout
[Risks and why]:  low
[String/UUID change made/needed]: none
Attachment #8671554 - Flags: approval-mozilla-aurora?
Comment on attachment 8671555 [details] [diff] [review]
backout 0bf012ea842f on aurora (gecko 43)

Approval Request Comment
[Feature/regressing bug #]: 1204614
[User impact if declined]: we would have to backport dependent fixes
[Describe test coverage new/current, TreeHerder]: backout
[Risks and why]:  low
[String/UUID change made/needed]: a change to nsIHttpChannelInternal.idl is made that reverts this to the way it is on <= 42
Attachment #8671555 - Flags: approval-mozilla-aurora?
Comment on attachment 8671554 [details] [diff] [review]
backout dfd4e61be062 on aurora (gecko 43)

Approved for aurora; backout of a feature that caused some regressions.
Attachment #8671554 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8671555 [details] [diff] [review]
backout 0bf012ea842f on aurora (gecko 43)

Approved for aurora; backout of a feature that caused some regressions.
Attachment #8671555 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I guess we should change the status flag here to "affected". A little confusing.
Target Milestone: mozilla43 → mozilla44
So there's nothing to uplift here at this point, right?
(In reply to Wes Kocher (:KWierso) from comment #19)
> So there's nothing to uplift here at this point, right?

right
Comment on attachment 8671554 [details] [diff] [review]
backout dfd4e61be062 on aurora (gecko 43)

Clearing the approval flags to get this out of uplift queues.
Attachment #8671554 - Flags: approval-mozilla-aurora+
Attachment #8671555 - Flags: approval-mozilla-aurora+
Depends on: 1247205
You need to log in before you can comment on or make changes to this bug.