Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: small fixes to compatibility layer #15473

Conversation

apapirovski
Copy link
Member

This is some assorted work on the compatibility layer that doesn't warrant its own PRs:

  • Expand argument validation through compat API
  • adjust behaviour of response.end to not throw if stream already closed to match http1
  • adjust behaviour of response.writeContinue to not throw if stream already closed and other very small tweaks
  • Add tests for added and fixed behaviour
  • Add tests for edge case behaviours of setTimeout, createPushResponse, destroy, end and trailers

I'll add some additional comments on the code itself, so see below for those.

As always, happy to implement any feedback. Thanks in advance~

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

Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.
@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Sep 19, 2017
const headers = this[kHeaders];
headers[HTTP2_HEADER_STATUS] = state.statusCode;
if (stream.finished === true)
options.endStream = true;
Copy link
Member Author

@apapirovski apapirovski Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find any reason that this would be needed. All our tests pass without it and if it is truly needed then there should be a test to cover it. Would appreciate any input!

(I'm referencing lines 520 & 521 above, 519 was just adjusted.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stream.finished check here is an optimization to avoid sending an unnecessary empty DATA frame.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell Right but what's the situation where stream.finished could be true in this particular code? We don't have any test cases that generate a situation like this (as you can see in the coverage since this code path is never hit).

Copy link
Member Author

@apapirovski apapirovski Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... is stream.finished even a thing? As far as I can tell, it's just always undefined. There's this.finished or stream._writableState.finished. Can they even respond in either of those cases?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

@mcollina
Copy link
Member

This PR is failing with the following error when trying to land:

=== release test-http2-compat-serverrequest-settimeout ===
Path: parallel/test-http2-compat-serverrequest-settimeout
(node:64823) ExperimentalWarning: The http2 module is an experimental API.
assert.js:44
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: function should not have been called
    at ServerHttp2Stream.mustNotCall (/Users/matteo/Repositories/node/test/common/index.js:565:12)
    at Object.onceWrapper (events.js:314:30)
    at emitNone (events.js:105:13)
    at ServerHttp2Stream.emit (events.js:207:7)
    at ServerHttp2Stream.emit (internal/http2/core.js:134:13)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)
Command: out/Release/node --expose-http2 /Users/matteo/Repositories/node/test/parallel/test-http2-compat-serverrequest-settimeout.js

@apapirovski
Copy link
Member Author

On it, looks like there's a race condition. Will refactor the test a bit.

@apapirovski
Copy link
Member Author

apapirovski commented Sep 20, 2017

Ok, changed a few places where we were checking for stream === undefined but should've really been checking for state.closed given the changes to when stream becomes undefined. This will need another review & CI run.

Thanks!

@mcollina
Copy link
Member

@apapirovski
Copy link
Member Author

Failures appear unrelated. On Windows, it's inspector/test-bindings and on Linux it's parallel/test-http-writable-true-after-close.

@mcollina
Copy link
Member

Landed as bc23681

@mcollina mcollina closed this Sep 21, 2017
mcollina pushed a commit that referenced this pull request Sep 21, 2017
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: #15473
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@apapirovski apapirovski deleted the patch-http2-compat-fixes-and-tests branch September 21, 2017 18:19
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: nodejs/node#15473
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
lukaszewczak pushed a commit to lukaszewczak/node that referenced this pull request Sep 23, 2017
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: nodejs#15473
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 25, 2017

This is landing clean in v8.x-staging but tests are failing. PTAL @apapirovski

@apapirovski
Copy link
Member Author

apapirovski commented Sep 25, 2017

@jasnell do you happen to have specific reference (/error message) to what's failing by any chance? This should be landing cleanly (incl tests) unless another commit is missing. Thanks!

@jasnell
Copy link
Member

jasnell commented Sep 25, 2017

I got it. There were some semver-major error code commits that this depended on. I've adjusted things in the cherry-picked commit. Should be good to go.

@apapirovski
Copy link
Member Author

Ah, awesome, thanks for sorting it! Sorry I couldn't be more help.

jasnell pushed a commit that referenced this pull request Sep 25, 2017
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: #15473
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: #15473
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: nodejs/node#15473
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants