Repository navigation
fix(server): Keep the idle limit on tunnels and proxied connections - #58
Merged
Merged
Conversation
A `CONNECT` tunnel gets a 10-minute idle limit on its raw socket. In a tunnel without TLS, the tunnel server of Node uses that socket. After a response, Node sets the limit to its keep-alive time (5 s and 1 s more), and at the next request it sets it to the `timeout` of the server, which was 0. So the limit went 600000 -> 6000 -> 0, and a second request that hung, such as one whose body or upstream never came, had no idle limit. Set `timeout` of the tunnel server to the idle limit, so Node sets the socket back to 10 minutes at each new request. A TLS tunnel keeps the limit of its raw socket as before, and its TLS socket gets the same limit. The new test reads the limit that the service sets on its end of a plain tunnel at the second request. It gets 0 on main and 600000 now. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com>
Node's requestTimeout stops when a request is read, and the main server had no timeout. A request that waited on a hung upstream, or a client that did not read its response, held the connection with no limit. Set the same 10-minute idle limit as the tunnel server. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two kinds of connection lost their 10-minute idle limit.
keepAliveTimeout+ 1 s (6 s), then toserver.timeout || 0at the next request. So a hung second request had no limit. The fix setstunnelServer.timeout.requestTimeoutstops once Node has read the request, so a hung upstream or a client that stops reading held the connection with no limit. The fix setsserver.timeoutto the same 10 minutes, because model calls can be slow before the first byte.TUNNEL_IDLE_MSis renamed toIDLE_MS. The README has an idle-limit bullet, and the keep-alive note now says 6 s.Tests: an
it.eachreads the timeout the service sets during a hung second request, for a tunnel and for an absolute-form connection. Both fail on main withexpected +0 to be 600000.pnpm checkpasses (116 tests).An independent review confirmed the root cause in Node 24's
_http_server.jsand reproduced the main-server gap.The docs cleanup PR is stacked on this one, because both edit the same README lines.
via @dcramer.
--
View Session [Sentry]