1204614 - use h2 per stream flow control with suspended channels

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: test for h2 per stream flow control

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: use h2 per stream flow control to deal with suspended channels

Comment 3

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb1d3692bcf

Comment 4

[Patrick McManus [:mcmanus]]

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 5

[u408661]

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?)

Comment 6

[u408661]

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.

Comment 7

[Patrick McManus [:mcmanus]]

> 
> 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.

Comment 8

[u408661]

(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 :)

Comment 9

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf012ea842fa811f1057355fb11259b7bc20487
bug 1204614 - use h2 per stream flow control to deal with suspended channels r=hurley

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd4e61be0624dba05fb0562f453fa93027a1663
bug 1204614 - test for h2 per stream flow control r=hurley

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0bf012ea842f
https://hg.mozilla.org/mozilla-central/rev/dfd4e61be062

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: backout dfd4e61be062 on aurora (gecko 43)

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: backout 0bf012ea842f on aurora (gecko 43)

Comment 13

[Patrick McManus [:mcmanus]]

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

Comment 14

[Patrick McManus [:mcmanus]]

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Approved for aurora; backout of a feature that caused some regressions.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8671555 [details] [diff] [review]
backout 0bf012ea842f on aurora (gecko 43)

Approved for aurora; backout of a feature that caused some regressions.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

I guess we should change the status flag here to "affected". A little confusing.

Comment 18

[Patrick McManus [:mcmanus]]

emote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/725efd9e78e0
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9eff48aef36a

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So there's nothing to uplift here at this point, right?

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to Wes Kocher (:KWierso) from comment #19)
> So there's nothing to uplift here at this point, right?

right

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Clearing the approval flags to get this out of uplift queues.