Skip to content

http2: callback valid check before closing request#19061

Closed
trivikr wants to merge 1 commit intonodejs:masterfrom
trivikr:close-callback-check
Closed

http2: callback valid check before closing request#19061
trivikr wants to merge 1 commit intonodejs:masterfrom
trivikr:close-callback-check

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
Copy link
Contributor

@cjihrig cjihrig Feb 28, 2018

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

@trivikr trivikr Feb 28, 2018

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

mcollina commented Feb 28, 2018

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
Copy link
Contributor

@cjihrig cjihrig Feb 28, 2018

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

@trivikr trivikr Feb 28, 2018

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
Contributor

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

apapirovski commented Mar 4, 2018

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 <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 close-callback-check branch March 4, 2018 19:43
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 6, 2018

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

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+2 more reviewers

@apapirovski apapirovski apapirovski left review comments

@Leko Leko Leko approved these changes

Reviewers whose approvals may not affect merge requirements

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.