Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
trivikr wants to merge 1 commit into from
Closed

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

trivikr wants to merge 1 commit into from

Conversation

@trivikr
Copy link
Member

@trivikrtrivikr commented Mar 8, 2018•
edited
Loading

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 [email protected]
Reviewed-By: Colin Ihrig [email protected]
Reviewed-By: James M Snell [email protected]
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Shingo Inoue [email protected]
Reviewed-By: Tobias Nießen [email protected]

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

@trivikr
http2: callback valid check before closing request
511c2a6
Do not close the request if callback is not a function, and throw ERR_INVALID_CALLBACK TypeError PR-URL: nodejs#19061Fixes: nodejs#18855 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@nodejs-github-botnodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Mar 8, 2018
@trivikrtrivikr mentioned this pull request Mar 8, 2018
Closed
3 tasks
@MylesBorinsMylesBorinsforce-pushed the v9.x-staging branch 3 times, most recently from d457b9d to 03c321aCompareMarch 20, 2018 11:56
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 20, 2018•
edited
Loading

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

MylesBorins commented Mar 21, 2018

=== 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
MemberAuthor

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.

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

targos commented Apr 4, 2018

Rebased and landed in 2bdf3ca

@targostargos closed this Apr 4, 2018
targos pushed a commit that referenced this pull request Apr 4, 2018
@trivikr@targos
http2: callback valid check before closing request
2bdf3ca
Do not close the request if callback is not a function, and throw ERR_INVALID_CALLBACK TypeError Backport-PR-URL: #19229 PR-URL: #19061Fixes: #18855 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@trivikrtrivikr 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
@trivikr@kjin
http2: callback valid check before closing request
dedf0c0
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#19061Fixes: nodejs#18855 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
@trivikr@MylesBorins
http2: callback valid check before closing request
af7c7cb
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: #19061Fixes: #18855 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@trivikr@MylesBorins
http2: callback valid check before closing request
82b56b6
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: #19061Fixes: #18855 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@trivikr@MylesBorins
http2: callback valid check before closing request
6d6e2e2
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: #19061Fixes: #18855 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@trivikr@MylesBorins@targos@nodejs-github-bot