Error handling for HPE_HEADER_OVERFLOW in HTTP server does not take into account socket re-use

[ramesius]

• Version: 12.19.0 and above
• Platform: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Tue Jan 12 22:13:05 PST 2021; root:xnu-6153.141.16~1/RELEASE_X86_64 x86_64

What steps will reproduce the bug?

Create a HTTP agent with keep-alive set to true, max sockets and max free sockets to 1 (for simplicity).
Send a request to a NodeJS HTTP server, resulting in a successful response.
Send a 2nd request, this time with a payload that will trigger HPE_HEADER_OVERFLOW (send a large header value, larger than max-header-size-limit).
Observe that the 2nd request has the socket destroyed, instead of receiving a HTTP 431.

The above steps are implemented in this gist: https://gist.github.com/ramesius/6d9fbd79d76ca36e555855275cf158c0 which is able to reproduce between node 12.18.4 and 12.19.0. For later versions it might require the example header to be increased in size.

How often does it reproduce? Is there a required condition?

Any socket that is re-used will not be able to respond with a HTTP 431.

What is the expected behavior?

A request that results in HPE_HEADER_OVERFLOW regardless of a one-use socket or a re-used socket should be able to return HTTP 431.

What do you see instead?

Any subsequent request over a re-used socket that triggers HPE_HEADER_OVERFLOW results in the socket being destroyed.
This is particularly troublesome when an application is behind any other service that keeps connections alive (for example a load balancer) for performance, the connections are unexpectedly closed which results in the service incorrectly interpreting what would have been a HTTP 431 as HTTP 503.

Additional information

The expected behavior was present in versions 12.18.4 and below.
Suspected buggy code: https://github.com/nodejs/node/blob/master/lib/_http_server.js#L658
When listening for clientError on the server we can also see that bytesWritten is > 0

This is the commit that introduced the changes

• e52cc24e31

[lpinca]

This might be a good reason to have #36964 in core.

cc: @ronag

[madmed88]

Any update or workaround for this?
This is causing a lot of 502 errors when running behind a load balancer and sending requests with very long headers.
upstream prematurely closed connection while reading response header from upstream