http2: respondWithFile() does not set headersSent to true

[trivikr]

• Version: v10.0.0-pre
• Platform: N/A
• Subsystem: http2

I came across this issue while writing unit tests for stream.respond() in PR #18861
The method respondWithFile() does not set state.flags, thus headersSent is still set to false:

node/lib/internal/http2/core.js

Lines 2267 to 2321 in 472cde6

	respondWithFile(path, headers, options) {
	if (this.destroyed || this.closed)
	throw new errors.Error('ERR_HTTP2_INVALID_STREAM');
	if (this.headersSent)
	throw new errors.Error('ERR_HTTP2_HEADERS_SENT');
	assertIsObject(options, 'options');
	options = Object.assign({}, options);
	if (options.offset !== undefined && typeof options.offset !== 'number')
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'offset',
	options.offset);
	if (options.length !== undefined && typeof options.length !== 'number')
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'length',
	options.length);
	if (options.statCheck !== undefined &&
	typeof options.statCheck !== 'function') {
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'statCheck',
	options.statCheck);
	}
	let streamOptions = 0;
	if (options.getTrailers !== undefined) {
	if (typeof options.getTrailers !== 'function') {
	throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
	'getTrailers',
	options.getTrailers);
	}
	streamOptions |= STREAM_OPTION_GET_TRAILERS;
	this[kState].getTrailers = options.getTrailers;
	}
	const session = this[kSession];
	debug(`Http2Stream ${this[kID]} [Http2Session ` +
	`${sessionName(session[kType])}]: initiating response`);
	this[kUpdateTimer]();
	headers = processHeaders(headers);
	const statusCode = headers[HTTP2_HEADER_STATUS] |= 0;
	// Payload/DATA frames are not permitted in these cases
	if (statusCode === HTTP_STATUS_NO_CONTENT ||
	statusCode === HTTP_STATUS_RESET_CONTENT ||
	statusCode === HTTP_STATUS_NOT_MODIFIED) {
	throw new errors.Error('ERR_HTTP2_PAYLOAD_FORBIDDEN', statusCode);
	}
	fs.open(path, 'r',
	afterOpen.bind(this, session, options, headers, streamOptions));
	}

This looks like a bug as both respond() and respondWithFD() set state.flags so that headersSent is set to true.

[antoine-amara]

If it's ok, I want to try to fix this issue, I reproduce the bug on my machine. It will be my first code contribution here after few contributions on documentation. But I think I can find it.

[trivikr]

Sure @antoine-amara, I know the locations of the fixes, can guide you on how to fix it and which tests to update.
I'm waiting for response from HTTP2 team to find out whether this is actually a bug.

[antoine-amara]

Ok thanks, I remain available to code a fix.

[joyeecheung]

cc @nodejs/http2

[mcollina]

This is indeed a bug. @antoine-amara Would you be able to send a PR?

[antoine-amara]

Yes, I find the function to update, I write the code, tests and I will made a PR as soon as possible. Thanks 👍

[antoine-amara]

Hello @trivikr @mcollina , I've got a fix but I don't know in which file I have to put my tests.

Can I post a PR and push the test later ?

[mcollina]

@antoine-amara I think you can add an assertion to test/parallel/test-http2-respond-file.js.

[antoine-amara]

Hello @trivikr @mcollina , I complete tests but my modifications break test-http2-respond-file-304 and I don't find how to fix it. my branch is here. I can make a PR if is needed.

[mcollina]

@antoine-amara open a PR and we will take a look!

[apapirovski]

respondWithFile has to call multiple different async methods, unlike respondWithFD, which means that checking for headersSent in a sync manner is not possible. I do not see a bug here.

[mcollina]

@apapirovski you are right! down the road respondWithFile will call processRespondWithFD https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L1907 which in turn will flip the headerSent state.

Very likely #19070 can be closed then. I'm sorry @antoine-amara.