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

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

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.

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

@apapirovski

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); 
        
           }

src: setting shuttingDown=true after validation

http2: setting shuttingDown=true after validation

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	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);
	}

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

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

Description

Activity

apapirovski commented on Sep 28, 2017

trivikr commented on Sep 29, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions