http2: fix compat stream read handling, add tests #15503

apapirovski · 2017-09-20T21:05:48Z

This PR is mostly about fixing #15491 but also includes some minor changes to improve overall reliability of the various stream events. Test cases for everything are included. (Some notes to follow directly in code.)

Thanks for reviewing!

cc @mcollina, @jasnell

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http2, test

apapirovski · 2017-09-20T21:06:26Z

lib/internal/http2/compat.js

    const onfinish = this[kFinish].bind(this);
    stream.on('streamClosed', onfinish);
    stream.on('finish', onfinish);
    this.on('pause', onRequestPause);
    this.on('resume', onRequestResume);
-    this.on('drain', onRequestDrain);


This isn't an event on Readable.

apapirovski · 2017-09-20T21:07:30Z

lib/internal/http2/compat.js

@@ -221,7 +222,7 @@ class Http2ServerRequest extends Readable {
  _read(nread) {
    const stream = this[kStream];
    if (stream !== undefined) {
-      stream.resume();
+      process.nextTick(() => stream.resume());


There's a race condition where pause can get evaluated in between resume being called and actually running (basically after resumeScheduled: true and before nextTick). This fixes it and passes all our tests (plus a new one that failed before).

we should not create a closure here, but rather use a top level function and pass the stream in there.

apapirovski · 2017-09-20T21:10:19Z

lib/internal/http2/compat.js

 function onStreamClosedRequest() {
  this[kRequest].push(null);
 }

+// TODO Http2Stream does not emit 'close'


I can remove these comments but I wanted it clear that these two don't run as things are. It's a bit confusing otherwise.

@jasnell can we remove those then? Or do you prefer if we add 'close' to Http2Stream?

I'm good with removing the comments.

oh lol... heh Matteo pointed out that I didn't understand the comments :-) This is true, I missed that.

I'm thinking we need to refactor this so that streamClosed and close are one and the same and that close is emitted appropriately. The one bit we'd have to account for is the code that is reported in streamClosed

I could be wrong but I think @mcollina is asking about removing the functions which the comments refer to. If a 'close' event will be added to Http2Stream at some point then they should be kept but if that's not going to happen then we can safely remove. too slow lol

apapirovski · 2017-09-20T21:29:11Z

lib/internal/http2/compat.js

-  if (this.writable) {
-    this.emit('close');
+  const response = this[kResponse];
+  if (response[kState].closed === false) {


writable is currently always true since Http2Stream doesn't emit a 'close' event but also this seems more correct, as far as I can reason about it and compare it to http1.

apapirovski · 2017-09-21T00:56:53Z

lib/internal/http2/compat.js

@@ -158,13 +160,12 @@ class Http2ServerRequest extends Readable {
    stream.on('end', onStreamEnd);
    stream.on('error', onStreamError);
    stream.on('close', onStreamClosedRequest);
-    stream.on('aborted', onStreamAbortedRequest.bind(this));


Any bind in here is expensive. I think only binding the most crucial things that are almost guaranteed to run (that being onfinish) is smart.

mcollina · 2017-09-21T07:14:49Z

lib/internal/http2/compat.js

-  res.writable = false;
-  res.emit('finish');
+  const response = this[kResponse];
+  response.writable = false;


I would remove setting writable = false. This would break pump. See #15029

mcollina

Good work, a couple of nits.

mcollina · 2017-09-21T07:15:30Z

lib/internal/http2/compat.js

@@ -221,7 +222,7 @@ class Http2ServerRequest extends Readable {
  _read(nread) {
    const stream = this[kStream];
    if (stream !== undefined) {
-      stream.resume();
+      process.nextTick(() => stream.resume());


we should not create a closure here, but rather use a top level function and pass the stream in there.

Handle edge case where stream pause is called between resume being called and actually evaluated. Other minor adjustments to avoid various edge cases around stream events. Add new tests that cover all changes. Fixes: nodejs#15491

apapirovski · 2017-09-21T13:10:00Z

Thanks for the review! Great spot re: the closure. All fixed now and ready for review/CI.

mcollina · 2017-09-21T16:14:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10183/

apapirovski · 2017-09-23T19:47:35Z

Sorry, just to confirm — anything I need to change or is this approved? Thanks! (Want to make sure this gets into 8.6.0 since without it people can't use HTTP2 compatibility layer very well, at least not for sending bigger amounts of data.)

Also, looks like CI was all good — failures are related to existing known issues reported elsewhere.

mcollina · 2017-09-25T07:39:44Z

@apapirovski I understood @jasnell would like to have https://github.com/nodejs/node/pull/15503/files#r140291319 sorted before landing.

IMHO leaving around functions that are not called is not a good thing.

We can land this as it is right now, and send another PR to fix the 'streamClosed' problem, I'm ok with that as well.

@jasnell what do you think?

mcollina

LGTM

apapirovski · 2017-09-25T11:10:21Z

I mean, this PR didn't change that behaviour (it just documents it) and it fixes a legitimate, serious bug. Would be nice to get it into 8.6.0. I can't imagine we'll have a resolution for standardizing streamClosed and close before then.

(I'm happy to take a look at that next btw.)

jasnell · 2017-09-25T20:26:34Z

I'm good with getting this landed now and included in 8.6.0, we can resolve the other issues separately

Handle edge case where stream pause is called between resume being called and actually evaluated. Other minor adjustments to avoid various edge cases around stream events. Add new tests that cover all changes. Fixes: #15491 PR-URL: #15503 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-09-25T20:34:43Z

Landed in c705f10

jasnell · 2017-09-25T20:36:09Z

cherry-picking into v8.x-staging next

Handle edge case where stream pause is called between resume being called and actually evaluated. Other minor adjustments to avoid various edge cases around stream events. Add new tests that cover all changes. Fixes: #15491 PR-URL: #15503 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-09-25T20:43:07Z

Landed in v8.x-staging...

Handle edge case where stream pause is called between resume being called and actually evaluated. Other minor adjustments to avoid various edge cases around stream events. Add new tests that cover all changes. Fixes: #15491 PR-URL: #15503 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

Handle edge case where stream pause is called between resume being called and actually evaluated. Other minor adjustments to avoid various edge cases around stream events. Add new tests that cover all changes. Fixes: nodejs/node#15491 PR-URL: nodejs/node#15503 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Sep 20, 2017

apapirovski commented Sep 20, 2017

View reviewed changes

apapirovski force-pushed the patch-http2-compat-events branch from 4ae899a to 05de543 Compare September 20, 2017 21:27

apapirovski commented Sep 20, 2017

View reviewed changes

apapirovski mentioned this pull request Sep 20, 2017

http2 - compat not working as http #15491

Closed

jasnell requested a review from mcollina September 20, 2017 22:24

apapirovski commented Sep 21, 2017

View reviewed changes

mcollina reviewed Sep 21, 2017

View reviewed changes

http2: fix compat stream read handling, add tests

e1af8b9

Handle edge case where stream pause is called between resume being called and actually evaluated. Other minor adjustments to avoid various edge cases around stream events. Add new tests that cover all changes. Fixes: nodejs#15491

apapirovski force-pushed the patch-http2-compat-events branch from 05de543 to e1af8b9 Compare September 21, 2017 13:09

ronag mentioned this pull request Sep 23, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

mcollina approved these changes Sep 25, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

jasnell closed this Sep 25, 2017

apapirovski deleted the patch-http2-compat-events branch September 25, 2017 20:44

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

apapirovski mentioned this pull request May 16, 2018

http2: fix end without read #20621

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: fix compat stream read handling, add tests #15503

http2: fix compat stream read handling, add tests #15503

apapirovski commented Sep 20, 2017 •

edited

Loading

apapirovski Sep 20, 2017

apapirovski Sep 20, 2017 •

edited

Loading

mcollina Sep 21, 2017

apapirovski Sep 20, 2017

mcollina Sep 21, 2017

jasnell Sep 21, 2017

jasnell Sep 21, 2017

apapirovski Sep 21, 2017 •

edited

Loading

apapirovski Sep 20, 2017

mcollina Sep 21, 2017

apapirovski Sep 21, 2017

mcollina Sep 21, 2017

mcollina left a comment

mcollina Sep 21, 2017

apapirovski commented Sep 21, 2017

mcollina commented Sep 21, 2017

apapirovski commented Sep 23, 2017 •

edited

Loading

mcollina commented Sep 25, 2017

mcollina left a comment

apapirovski commented Sep 25, 2017 •

edited

Loading

jasnell commented Sep 25, 2017

jasnell commented Sep 25, 2017

jasnell commented Sep 25, 2017

jasnell commented Sep 25, 2017

http2: fix compat stream read handling, add tests #15503

http2: fix compat stream read handling, add tests #15503

Conversation

apapirovski commented Sep 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

apapirovski Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Sep 21, 2017

mcollina commented Sep 21, 2017

apapirovski commented Sep 23, 2017 • edited Loading

mcollina commented Sep 25, 2017

mcollina left a comment

Choose a reason for hiding this comment

apapirovski commented Sep 25, 2017 • edited Loading

jasnell commented Sep 25, 2017

jasnell commented Sep 25, 2017

jasnell commented Sep 25, 2017

jasnell commented Sep 25, 2017

apapirovski commented Sep 20, 2017 •

edited

Loading

apapirovski Sep 20, 2017 •

edited

Loading

apapirovski Sep 21, 2017 •

edited

Loading

apapirovski commented Sep 23, 2017 •

edited

Loading

apapirovski commented Sep 25, 2017 •

edited

Loading