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: check if callback is valid before closing the request? #18855

Closed
trivikr opened this issue Feb 18, 2018 · 7 comments
Closed

http2: check if callback is valid before closing the request? #18855

trivikr opened this issue Feb 18, 2018 · 7 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@trivikr
Copy link
Member

trivikr commented Feb 18, 2018

  • Version: master
  • Platform: N/A
  • Subsystem: http2

I came across this issue while writing unit tests for req.close(code) in PR #18854
The function currently checks if callback is valid after performing close operation

if (callback !== undefined) {
if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');
this.once('close', callback);
}

In other functions (for example ping(), settings() etc) the validity for callback is checked before function performs it's operation:

if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');

if (callback && typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');

Is there a reason why req.close(code) performs close operation even if callback is invalid?

@joyeecheung joyeecheung added the http2 Issues or PRs related to the http2 subsystem. label Feb 20, 2018
@joyeecheung
Copy link
Member

cc @nodejs/http2

@mcollina
Copy link
Member

I think this is a bug, would you mind sending a PR?

@trivikr
Copy link
Member Author

trivikr commented Feb 26, 2018

Sure, I'll post a PR in a day or two

@trivikr
Copy link
Member Author

trivikr commented Feb 28, 2018

Blocked on #19057 and #18938

@mcollina
Copy link
Member

@trivikr why is this blocked by those? Can you open the PR in the meanwhile?

@trivikr
Copy link
Member Author

trivikr commented Feb 28, 2018

I can make the code change and test it by running just single test as defined in https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#step-6-test

However, since coverage is failing on my machine and also at coverage.nodejs.org I would like to wait for that fix before posting a PR.

@trivikr
Copy link
Member Author

trivikr commented Feb 28, 2018

PR posted at #19061

trivikr added a commit to trivikr/node that referenced this issue Feb 28, 2018
Do not close the request if callback is not a function, and
throw ERR_INVALID_CALLBACK TypeError

Fixes: nodejs#18855
trivikr added a commit to trivikr/node that referenced this issue Mar 8, 2018
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]>
targos pushed a commit that referenced this issue 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 <[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]>
kjin pushed a commit to kjin/node that referenced this issue 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 <[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 issue 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 <[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]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
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 issue 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 <[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 issue 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 <[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
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants