fix: gracefully close connections on errors while piping responses

[aleortega]

This PR handles errors while piping responses to prevent the connection from hanging until it times out.

[aleortega]

Hey, we spotted this in a fork we did of this server. We are working to implement support for HTTP/2 protocol capabilities, such as socket re-use, to enhance performance on the client side. To do so, we have aligned the server timeouts to be above the timeouts of our infrastructure (CloudFlare and AWS ALB in our case), and this version wouldn't allow us to do so.

After making that change, we spotted that some piped responses weren't being closed as expected on errors, and they were timing out (at 70 seconds instead of 5 seconds as this version does).

Hope it is useful!

[menduz]

You did send data. Probably the .text() should receive it instead of failing, you also did send response headers.

Have you considered .end() instead of .destroy(err)? also http/1.1 (and newer) support keep-alive connections, probably the .end will be nicer to connection pools

[aleortega]

Hey, thanks for reviewing. To be honest, I am still not sure what approach we should take here. This is what I see:

• .end() will prevent .text() from throwing, but the client will have truncated data on their end, unexpectedly masking the error
• .destroy() will make .text() throw, but the clients could be aware of the error and act accordingly

HTTP/2 handles .destroy() correctly according to what I've read because it just acts over the specific handled stream (RST_STREAM signal) and not over the whole socket connection.

Recently, I spotted this change in NodeJS.

Basically, NodeJS used to raise an error whenever promisedHeader.Content-Length !== actualContentLength, preventing the .end() masking the error, as far as I understand. Now it is the client's responsibility to check that (realized if the body has been truncated).

Having said that 👆 I think .end() would be the way to go as you proposed, because:

• Clients need to start checking content-length on their own, making the concern of truncated data for /1.1 unfounded
• Implementing the .destroy() will provoke a breaking change since now the .text() would throw

The only downside I see is that we will still mask the error for chunked encoding. We can iterate over this case if you want!

I just pushed a new commit aligning the approach to the feedback!

[menduz]

Changeset looks good, I'm thinking about who catches the error from server side. Either it is a logger or tracing component. The client can decide what to do with the received data (content-length checks or not), but we may be hiding an error in the server. Do you have clarity about that?

[aleortega]

Oh yes, I've added access to the logger at that level, and now we log the error whenever it occurs. Didn't spot any tracing component, please let me know if I am missing something

Will keep this PR open for a few hours in case feedback arrives, thank you!