Skip to content

Tell the client the connection is closing after each response - #212

Open
arpitjain099 wants to merge 1 commit into
coder:mainfrom
arpitjain099:fix/signal-connection-close
Open

arpitjain099 wants to merge 1 commit into
coder:mainfrom
arpitjain099:fix/signal-connection-close

Conversation

@arpitjain099

Copy link
Copy Markdown

Fixes #211.

handleHTTPConnection and handleTLSConnection each read one request and close the connection in a defer, but neither response says so. The upstream Connection: keep-alive is not passed through either, so a client that pools sockets, Node's https.Agent({keepAlive: true}) in the report, keeps the socket and the next write lands on a connection the proxy has already gone away from. That is the alternating socket hang up in the issue, and it is why code-server --install-extension fails outright and npm install crawls.

RFC 9112 section 9.6 puts that on the sender: if you are going to close, say so. The forwarded response now drops any upstream Connection header and sets resp.Close, so Response.Write emits Connection: close, and the blocked 403 does the same.

This is the first of the two options in the issue. Actually keeping connections alive, the second option, is the better outcome for throughput, but it means holding the connection open and looping over requests in both handlers, which is a larger change and a design call for you rather than something to slip into a bug fix.

The test starts a local backend, goes through the proxy for one allowed and one blocked request, and asserts resp.Close on both. It fails on main with "response must tell the client the connection is closing". go test -race ./proxy/ passes.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@a-palamarchuk

Copy link
Copy Markdown

Thanks for tracking this down; the diagnosis in #211 and the RFC 9112 reasoning both look right to me for the transparent paths.

One concern: forwardRequest and writeBlockedResponse are also used by the CONNECT tunnel loop (handleCONNECTTunnel in proxy/connect.go). That loop does serve multiple requests per connection, so with this change every response inside a tunnel carries Connection: close. Clients then drop the tunnel after each response, and every HTTPS request pays for a new CONNECT and TLS handshake with the proxy. That's the explicit-proxy mode landjail uses (it sets HTTPS_PROXY in landjail/child.go), so this would trade the nsjail bug for a keep-alive regression under landjail.

I checked this with a throwaway test: 5 sequential HTTPS requests through the explicit proxy with one Go client, counting new connections with httptrace:

main:  HTTPS via proxy: 5 requests, 1 new connections
#212:  HTTPS via proxy: 5 requests, 5 new connections

The existing TestMultipleRequestsOverExplicitCONNECT still passes because it writes raw requests and ignores the Connection header, and the new test only covers the plain-HTTP explicit path, which is one-shot.

A suggestion that keeps the fix and avoids the regression: let the caller say whether the connection closes after this response, since only the caller knows. For example, add a closeAfter bool parameter to processHTTPRequest and pass it through to forwardRequest and writeBlockedResponse:

  • handleHTTPConnection and handleTLSConnection pass true (they close in their defer).
  • handleCONNECTTunnel passes false.
  • The responses set resp.Close = closeAfter.

Removing the upstream Connection header still makes sense unconditionally, since it's hop-by-hop.

This also affects #216, which makes forwardRequest return a 502 when the upstream can't be reached. That 502 should follow the same flag; whichever of the two PRs lands second can thread it through.

A test case that would catch this: establish a tunnel with establishExplicitCONNECT, send a request, and assert that resp.Close is false and that a second request on the same tunnel succeeds.

Happy to help with that change or the test if useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keep-alive connections are closed without Connection: close, breaking socket reuse (code-server extension install fails; npm ~90x slower)

2 participants