Review requested:

• @nodejs/http
• @nodejs/net

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 Are you referring to the test, right?

@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.

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.

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.

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)

@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.

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

LGTM.

@nodejs/http Are we good on this?

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

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

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

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

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

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

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

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