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.

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

http2: callback valid check before closing request #19061

Closed
wants to merge 1 commit into from
Closed

http2: callback valid check before closing request #19061

wants to merge 1 commit into from

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented Feb 28, 2018

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

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Feb 28, 2018
@trivikr
Copy link
Member Author

trivikr commented Feb 28, 2018

Fixes #18855

@trivikr trivikr mentioned this pull request Feb 28, 2018
Closed
cjihrig
cjihrig reviewed Feb 28, 2018
View reviewed changes
lib/internal/http2/core.js Outdated
@@ -1764,6 +1764,8 @@ class Http2Stream extends Duplex {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'code', 'number');
if (code < 0 || code > kMaxInt)
throw new errors.RangeError('ERR_OUT_OF_RANGE', 'code');
if (callback && typeof callback !== 'function')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the first part of this line to callback !== undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

mcollina
mcollina approved these changes Feb 28, 2018
View reviewed changes
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

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

@trivikr trivikr force-pushed the close-callback-check branch from 55ed924 to d202b72 Compare February 28, 2018 15:53
cjihrig
cjihrig approved these changes Feb 28, 2018
View reviewed changes
test/parallel/test-http2-client-rststream-before-connect.js Outdated
@@ -28,6 +28,18 @@ server.listen(0, common.mustCall(() => {
);
assert.strictEqual(req.closed, false);

[true, 1, {}, [], 'test'].forEach((notFunction) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should have said this before: can you add null here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@trivikr
http2: callback valid check before closing request
4bb2142 
Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

Fixes: nodejs#18855
@trivikr trivikr force-pushed the close-callback-check branch from d202b72 to 4bb2142 Compare February 28, 2018 16:08
@mcollina
Copy link
Member

mcollina commented Mar 1, 2018

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
apapirovski
apapirovski reviewed Mar 2, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

I think it's debatable whether this change is correct or not. The callback is not a requirement to closing a stream and this can introduce a resource leak for someone that might be handling this type of error via a domain or an uncaughtException (as dubious as that is). It's also worth noting that the existing version matches other usage in the codebase where a callback is only provided as an event notification mechanism.

@mcollina
Copy link
Member

mcollina commented Mar 2, 2018

@apapirovski the code is called synchronously from user code. If there a wrong value as a callback they need to crash. The alternative here is that if it's not a function it gets skipped,
as its done in streams (https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L273-L274).

I think we should validate synchronously when close() is called, and in case validation of that parameter fail we might decide either to throw or to replace it with a function() {}. I think throwing leads to better code.

Anyway, are you objecting?

@apapirovski
Copy link
Member

Landed in caaf7e3

@apapirovski apapirovski closed this Mar 4, 2018
apapirovski pushed a commit that referenced this pull request Mar 4, 2018
@trivikr @apapirovski
http2: callback valid check before closing request
caaf7e3 
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]>
@trivikr trivikr deleted the close-callback-check branch March 4, 2018 19:43
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

trivikr added a commit to trivikr/node that referenced this pull request Mar 8, 2018
@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#19061
Fixes: 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]>
@trivikr trivikr mentioned this pull request Mar 8, 2018
Closed
4 tasks
@trivikr
Copy link
Member Author

trivikr commented Mar 8, 2018

@MylesBorins v9.x backport PR created at #19229

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 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: #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]>
@targos targos mentioned this pull request Apr 4, 2018
Merged
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#19061
Fixes: 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: #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]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@trivikr @MayaLekova
http2: callback valid check before closing request
4257473 
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 <[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: #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]>
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: #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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Leko Leko Leko approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.