http: unify header treatment

[marco-ippolito]

resolves: #46395
I'm not sure what the test test/parallel/test-http-server-response-standalone.js impacted by this change was trying to achieve @Trott

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[Trott]

I'm not sure what the test test/parallel/test-http-server-response-standalone.js impacted by this change was trying to achieve @Trott

It was added in #14387 by @mcollina so maybe he can add context if that PR doesn't supply the information you need.

[climba03003]

It doesn't look right to me.
The header should be process before ._send, so it can be ensure either merge with data and pending for first write always receive the same byte.

[ShogunPanda]

@climba03003 Are you referring to the test, right?

[climba03003]

@climba03003 Are you referring to the test, right?

I means the implementation.
Combining header and data is used to increase the throughput.
Remove !encoding is a de-optimization since most of the people doesn't specify encoding.

The trade-off is process the header with consistence encoding before combine to data.
Add a bit of processing time, but still combine header and data in most of the case.

node/lib/_http_outgoing.js

Lines 438 to 587 in a2a954b

	function _storeHeader(firstLine, headers) {
	// firstLine in the case of request is: 'GET /index.html HTTP/1.1\r\n'
	// in the case of response it is: 'HTTP/1.1 200 OK\r\n'
	const state = {
	connection: false,
	contLen: false,
	te: false,
	date: false,
	expect: false,
	trailer: false,
	header: firstLine
	};
	if (headers) {
	if (headers === this[kOutHeaders]) {
	for (const key in headers) {
	const entry = headers[key];
	processHeader(this, state, entry[0], entry[1], false);
	}
	} else if (ArrayIsArray(headers)) {
	if (headers.length && ArrayIsArray(headers[0])) {
	for (let i = 0; i < headers.length; i++) {
	const entry = headers[i];
	processHeader(this, state, entry[0], entry[1], true);
	}
	} else {
	if (headers.length % 2 !== 0) {
	throw new ERR_INVALID_ARG_VALUE('headers', headers);
	}
	for (let n = 0; n < headers.length; n += 2) {
	processHeader(this, state, headers[n + 0], headers[n + 1], true);
	}
	}
	} else {
	for (const key in headers) {
	if (ObjectPrototypeHasOwnProperty(headers, key)) {
	processHeader(this, state, key, headers[key], true);
	}
	}
	}
	}
	let { header } = state;
	// Date header
	if (this.sendDate && !state.date) {
	header += 'Date: ' + utcDate() + '\r\n';
	}
	// Force the connection to close when the response is a 204 No Content or
	// a 304 Not Modified and the user has set a "Transfer-Encoding: chunked"
	// header.
	//
	// RFC 2616 mandates that 204 and 304 responses MUST NOT have a body but
	// node.js used to send out a zero chunk anyway to accommodate clients
	// that don't have special handling for those responses.
	//
	// It was pointed out that this might confuse reverse proxies to the point
	// of creating security liabilities, so suppress the zero chunk and force
	// the connection to close.
	if (this.chunkedEncoding && (this.statusCode === 204 ||
	this.statusCode === 304)) {
	debug(this.statusCode + ' response should not use chunked encoding,' +
	' closing connection.');
	this.chunkedEncoding = false;
	this.shouldKeepAlive = false;
	}
	// keep-alive logic
	if (this._removedConnection) {
	// shouldKeepAlive is generally true for HTTP/1.1. In that common case,
	// even if the connection header isn't sent, we still persist by default.
	this._last = !this.shouldKeepAlive;
	} else if (!state.connection) {
	const shouldSendKeepAlive = this.shouldKeepAlive &&
	(state.contLen || this.useChunkedEncodingByDefault || this.agent);
	if (shouldSendKeepAlive && this.maxRequestsOnConnectionReached) {
	header += 'Connection: close\r\n';
	} else if (shouldSendKeepAlive) {
	header += 'Connection: keep-alive\r\n';
	if (this._keepAliveTimeout && this._defaultKeepAlive) {
	const timeoutSeconds = MathFloor(this._keepAliveTimeout / 1000);
	let max = '';
	if (~~this._maxRequestsPerSocket > 0) {
	max = `, max=${this._maxRequestsPerSocket}`;
	}
	header += `Keep-Alive: timeout=${timeoutSeconds}${max}\r\n`;
	}
	} else {
	this._last = true;
	header += 'Connection: close\r\n';
	}
	}
	if (!state.contLen && !state.te) {
	if (!this._hasBody) {
	// Make sure we don't end the 0\r\n\r\n at the end of the message.
	this.chunkedEncoding = false;
	} else if (!this.useChunkedEncodingByDefault) {
	this._last = true;
	} else if (!state.trailer &&
	!this._removedContLen &&
	typeof this._contentLength === 'number') {
	header += 'Content-Length: ' + this._contentLength + '\r\n';
	} else if (!this._removedTE) {
	header += 'Transfer-Encoding: chunked\r\n';
	this.chunkedEncoding = true;
	} else {
	// We should only be able to get here if both Content-Length and
	// Transfer-Encoding are removed by the user.
	// See: test/parallel/test-http-remove-header-stays-removed.js
	debug('Both Content-Length and Transfer-Encoding are removed');
	}
	}
	// Test non-chunked message does not have trailer header set,
	// message will be terminated by the first empty line after the
	// header fields, regardless of the header fields present in the
	// message, and thus cannot contain a message body or 'trailers'.
	if (this.chunkedEncoding !== true && state.trailer) {
	throw new ERR_HTTP_TRAILER_INVALID();
	}
	this._header = header + '\r\n';
	this._headerSent = false;
	// Wait until the first body chunk, or close(), is sent to flush,
	// UNLESS we're sending Expect: 100-continue.
	if (state.expect) this._send('');
	}
	function processHeader(self, state, key, value, validate) {
	if (validate)
	validateHeaderName(key);
	if (ArrayIsArray(value)) {
	if (
	(value.length < 2 || !isCookieField(key)) &&
	(!self[kUniqueHeaders] || !self[kUniqueHeaders].has(StringPrototypeToLowerCase(key)))
	) {
	// Retain for(;;) loop for performance reasons
	// Refs: https://github.com/nodejs/node/pull/30958
	for (let i = 0; i < value.length; i++)
	storeHeader(self, state, key, value[i], validate);
	return;
	}
	value = ArrayPrototypeJoin(value, '; ');
	}
	storeHeader(self, state, key, value, validate);
	}

[ShogunPanda]

I knew about the optimization. I was double checking.
I guess when encoding it's not set we have to make sure we correctly encode the headers. Hoping it does not result in big performance loss.

[climba03003]

I guess when encoding it's not set we have to make sure we correctly encode the headers. Hoping it does not result in big performance loss.

Inside the code, node.js is always expecting the header is latin1 encoding.
So, we can do something like Buffer.from(headerValue, 'latin').toString('utf-8') in _storeHeader.
Maybe there is another approach which is more performant.

In this case, we can expect the non-string branch always utf-8. Since we do the transformation already.
In string branch, latin1 or utf-8 encoding doesn't really matter. Since the byte already changed.

// from utf-8 to latin1
const first = Buffer.from('å', 'utf8').toString('latin1')
console.log(first, Buffer.from(first)) // <Buffer c3 83 c2 a5>

// here is what should be done before hand
const second = Buffer.from('å', 'latin1').toString('utf8')
console.log(second, Buffer.from(second)) // <Buffer ef bf bd>

// when data is latin1
const third = Buffer.from('ef bf bd', 'hex')
console.log(third, Buffer.from(third, 'latin1')) // <Buffer ef>
// when data is utf-8 or no encoding
const fouth = Buffer.from('ef bf bd', 'hex')
console.log(fouth, Buffer.from(fouth, 'utf8')) // <Buffer ef>

Edit: hmm, buffer truncated doesn't seems correct.

[climba03003]

The problem is actually cause by content-disposition allow latin1 but other header is always restricted to ASCII.

Maybe special handling of content-disposition in _storeHeader should do the trick without massive change.
Same as nodejs/undici#1903 (comment)

[marco-ippolito]

@climba03003 I've changed it to check inside processHeader if the header is content-disposition and there is content-length it encodes it in latin1. It will not impact performance but it feels more like a workaround rather than a full solution.

[ShogunPanda]

The RFC itself is a special case so I guess we have to act in "workaround way".

LGTM.

[ShogunPanda]

@nodejs/http Are we good on this?

[mcollina]

lgtm

[marco-ippolito]

I think this is going to be a breaking change wdyt? @ronag @Trott

[ronag]

I think it's more of a bug fix than a breaking change...

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49772/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49778/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49817/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49821/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/49825/

[nodejs-github-bot]

Landed in 3fd4343

[ArsalanDotMe]

This PR introduced a bug #50978 and there is an associated PR #50977 to fix it. Please review when possible.