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

[trivikr]

• 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

node/lib/internal/http2/core.js

Lines 1760 to 1764 in 472cde6

	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:

node/lib/internal/http2/core.js

Lines 954 to 955 in 472cde6

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

node/lib/internal/http2/core.js

Lines 1044 to 1045 in 472cde6

	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]

cc @nodejs/http2

[mcollina]

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

[trivikr]

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

[trivikr]

Blocked on #19057 and #18938

[mcollina]

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

[trivikr]

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]

PR posted at #19061