http2: fix compat stream read handling, add tests by apapirovski · Pull Request #15503 · nodejs/node · GitHub

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

This isn't an event on Readable.

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.

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

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.

👍

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.

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

Good work, a couple of nits.

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

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

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

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.

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

LGTM

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

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 <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in c705f10

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 <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

http2: fix compat stream read handling, add tests by apapirovski · Pull Request #15503 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

http2: fix compat stream read handling, add tests#15503apapirovski wants to merge 1 commit intonodejs:masternodejs/node:masterfrom apapirovski:patch-http2-compat-eventsapapirovski/node:patch-http2-compat-eventsCopy head branch name to clipboard

Conversation

apapirovski commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Sep 21, 2017

Uh oh!

mcollina commented Sep 21, 2017

Uh oh!

apapirovski commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Sep 25, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 25, 2017

Uh oh!

jasnell commented Sep 25, 2017

Uh oh!

jasnell commented Sep 25, 2017

Uh oh!

jasnell commented Sep 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

http2: fix compat stream read handling, add tests#15503
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-http2-compat-events

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