feat: Clean up the incoming object if the request is not completely finished.

[usualoma]

fixes #246

What was the situation?

If the request body was not read to the end (the timing varies depending on whether it is http/https/http2 or keepalive is enabled), the node-server did not close the socket.In this case, the socket is typically closed on the client side, so this is not an issue for most clients. However, if the application does not read the entire body and returns a 2xx status code, or if the client does not close the connection even when a 2xx status code is returned, server-side resources may not be released.

Problem classification

incoming.on('end') is not emitted

First of all, if incoming.on(‘end’) is not emitted (i.e., it has not been read to the end), the socket will not be closed automatically. (It can be closed by disconnecting from the client side.)

The following section addresses this issue.This processing must be limited to cases where there is a real problem, otherwise keepalive requests will fail.

https://github.com/honojs/node-server/pull/252/files#diff-0a9945c399ef4b66209b330f33399ac079477f017039f62d33e7d8296163469dR245-R254

Cancelled

For some reason, when cancel() is executed on a ReadableStream created with Readable.toWeb(), the socket is no longer closed automatically.

The following section addresses this issue.

https://github.com/honojs/node-server/pull/252/files#diff-b1a2ab03a3672e105afeaab6615ad625365cd8459d592a840d925f132af62b35R88-R104

http2

In http2, it seems that outgoing.on(‘close') is not emitted every time. Also, incoming.destroy() alone does not close the socket, and outgoing.destroy() must be called explicitly.

https://github.com/honojs/node-server/pull/252/files#diff-0a9945c399ef4b66209b330f33399ac079477f017039f62d33e7d8296163469dR223-R224

Behavior in v18

In Node.js v18, I couldn't get it to end with the “partially-consumed” pattern no matter what I tried.

https://github.com/honojs/node-server/pull/252/files#diff-d186195e48b9676c4bd921f08ef91e300527e6947e11a36bafcf90122e9596e0R163-R224

Further investigation may reveal a solution, but since v18 has already reached end-of-life (LTS), I do not believe it is necessary to pursue this further. (For node-server as a whole, it may be advisable to remove v18 in the next major version.)

New option “autoCleanupIncoming”

The feature added in this PR causes overhead, especially in parts that wrap the body. (I don't think it's that big of a deal, though.)
https://github.com/honojs/node-server/pull/252/files#diff-b1a2ab03a3672e105afeaab6615ad625365cd8459d592a840d925f132af62b35R88-R104

If the application is placed behind nginx or similar, request body size limits and timeouts are often set on the web server, so the functionality added in this PR is not necessary in such environments.

By setting "autoCleanupIncoming" to false, you can avoid overhead and maintain compatibility with previous versions.

[usualoma]

Hi @yusukebe
Would you please review this?

[yusukebe]

Hi @usualoma ! Thank you for the hard work!

I left a comment. Other than that, I think it looks good.

Further investigation may reveal a solution, but since v18 has already reached end-of-life (LTS), I do not believe it is necessary to pursue this further. (For node-server as a whole, it may be advisable to remove v18 in the next major version.)

Indeed, we can end support for Node.js v18 in the next major version. I've created issue #253

[yusukebe]

LGTM!

[yusukebe]

@usualoma

It looks good to me. Thank you very much!