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

[v9.x backport] http2: callback valid check before closing request #19229

Closed
wants to merge 1 commit into from

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented Mar 8, 2018

Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

PR-URL: #19061
Fixes: #18855
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de
Reviewed-By: Shingo Inoue leko.noor@gmail.com
Reviewed-By: Tobias Nießen tniessen@tnie.de

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

PR-URL: nodejs#19061
Fixes: nodejs#18855
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Mar 8, 2018
@MylesBorins MylesBorins force-pushed the v9.x-staging branch 3 times, most recently from d457b9d to 03c321a Compare March 20, 2018 11:56
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 20, 2018

the test from this PR appears to be failing. Can you please test locally to verify

edit: changed above phrasing... did not mean to imply "can you please test before pushing" rather "please test to verify it is broken"

@MylesBorins
Copy link
Contributor

=== release test-http2-client-rststream-before-connect ===
Path: parallel/test-http2-client-rststream-before-connect
(node:3048) ExperimentalWarning: The http2 module is an experimental API.
assert.js:49
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: 'The "code" argument is out of range' === 'The value of "code" is out of range.'
    at Object.innerFn (/Users/mborins/code/node/v9.x/test/common/index.js:705:16)
    at expectedException (assert.js:252:19)
    at Function.throws (assert.js:297:16)
    at Object.expectsError (/Users/mborins/code/node/v9.x/test/common/index.js:727:12)
    at Http2Server.server.listen.common.mustCall (/Users/mborins/code/node/v9.x/test/parallel/test-http2-client-rststream-before-connect.js:21:10)
    at Http2Server.<anonymous> (/Users/mborins/code/node/v9.x/test/common/index.js:477:15)
    at Object.onceWrapper (events.js:272:13)
    at Http2Server.emit (events.js:180:13)
    at emitListeningNT (net.js:1372:10)
    at process._tickCallback (internal/process/next_tick.js:114:19)
Command: out/Release/node /Users/mborins/code/node/v9.x/test/parallel/test-http2-client-rststream-before-connect.js
[00:00|% 100|+   0|-   1]: Done

@targos
Copy link
Member

targos commented Mar 24, 2018

ping @trivikr. It looks like the message test needs to be adapted to v9.x

@trivikr
Copy link
Member Author

trivikr commented Mar 24, 2018

The test is successful in my local workspace

And the value for ERR_OUT_OF_RANGE hasn't changed in v9.x-staging branch https://github.com/nodejs/node/blob/v9.x-staging/lib/internal/errors.js#L689

However, it looks like the errors were updated in master branch in commit 6e1c25c
This is going to impact multiple tests https://github.com/nodejs/node/search?p=2&q=ERR_OUT_OF_RANGE&type=&utf8=%E2%9C%93

cc: @BridgeAR

@targos
Copy link
Member

targos commented Mar 24, 2018

@trivikr The test does not check for the right error message (it is different between master and v9.x). It's weird that it passes in your local workspace.

@MylesBorins MylesBorins force-pushed the v9.x-staging branch 2 times, most recently from 305fe4c to 4844a26 Compare March 28, 2018 16:23
@targos
Copy link
Member

targos commented Apr 4, 2018

Rebased and landed in 2bdf3ca

@targos targos closed this Apr 4, 2018
targos pushed a commit that referenced this pull request Apr 4, 2018
Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

Backport-PR-URL: #19229
PR-URL: #19061
Fixes: #18855
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@trivikr trivikr deleted the backport-19061-to-v9.x branch April 4, 2018 15:01
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

Backport-PR-URL: nodejs#19229
PR-URL: nodejs#19061
Fixes: nodejs#18855
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

Backport-PR-URL: #19229
Backport-PR-URL: #20456
PR-URL: #19061
Fixes: #18855
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

Backport-PR-URL: #19229
Backport-PR-URL: #20456
PR-URL: #19061
Fixes: #18855
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

Backport-PR-URL: #19229
Backport-PR-URL: #20456
PR-URL: #19061
Fixes: #18855
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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.

4 participants