Should invalid options for http2.session.shutdown() be checked before state shuttingDown is updated to true?

[trivikr]

I was writing unit tests for invalid options passed to http2.session.shutdown() as part of #14985 for

node/lib/internal/http2/core.js

Lines 999 to 1024 in 2f2f1cf

	if (options.opaqueData !== undefined &&
	!isUint8Array(options.opaqueData)) {
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'opaqueData',
	options.opaqueData);
	}
	if (type === NGHTTP2_SESSION_SERVER &&
	options.graceful !== undefined &&
	typeof options.graceful !== 'boolean') {
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'graceful',
	options.graceful);
	}
	if (options.errorCode !== undefined &&
	typeof options.errorCode !== 'number') {
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'errorCode',
	options.errorCode);
	}
	if (options.lastStreamID !== undefined &&
	(typeof options.lastStreamID !== 'number' ||
	options.lastStreamID < 0)) {
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'lastStreamID',
	options.lastStreamID);
	}

Should we check for invalid options before setting this[kState].shuttingDown to true?

node/lib/internal/http2/core.js

Line 987 in 2f2f1cf

this[kState].shuttingDown = true;

Currently, if there are invalid options:

• this[kState].shuttingDown is set to true
• an exception is thrown
• the session is not shut down.

When shutdown is called for the second time, it returns because of the following check

node/lib/internal/http2/core.js

Lines 983 to 984 in 2f2f1cf

	if (this[kState].shutdown || this[kState].shuttingDown)
	return;

In this case, the only way to end the session is to destroy it.

[apapirovski]

Good spot! It should definitely be set only after the arguments are validated. Please include it with your PR for the tests, if possible.

[trivikr]

Thanks @apapirovski
I'll include the fix for this issue with my tests.

I'll set this[kState].shuttingDown to true just after checking for invalid options, and before the callback check:

node/lib/internal/http2/core.js

Lines 1026 to 1028 in 2f2f1cf

	if (callback) {
	this.on('shutdown', callback);
	}