From 9ef8886630622186b79943cbe80ef8399a4e4777 Mon Sep 17 00:00:00 2001 From: Teo Koon Peng Date: Thu, 5 Oct 2023 21:57:25 +0800 Subject: [PATCH] Fix trying to close nil connection (#233) I just learned that the behavior of multiple Close is undefined so we probably should avoid calling it multiple times https://github.com/golang/go/blob/5152be3959d4aa273932c12da971d14e7f84405d/src/io/io.go#L103-L109. I think there are 3 scenarios here, either backend closes the connection, the remote (relay-server) closes, or the remote returns an error. 1. If the backend closes, either streamToBackend or streamBytes can close hresp.Body, but streamToBackend only runs for websocket connections. 2. If the remote closes, streamToBackend is the only one that knows about it. 3. If the remote returns an error, streamToBackend is the only one that knows about it. It makes sense to close it in streamToBackend, but streamToBackend only runs for websocket connections. It works if we also close in streamBytes, but that leads to a racey condition and causes us to close multiple times. It's probably better if we close it in the else branch of line ~588 (if *resp.StatusCode == http.StatusSwitchingProtocols {), this way we ensure that we only ever call Close() once. --------- Signed-off-by: Teo Koon Peng --- src/go/cmd/http-relay-client/client/client.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/go/cmd/http-relay-client/client/client.go b/src/go/cmd/http-relay-client/client/client.go index 1ee33b45..ce9f3005 100644 --- a/src/go/cmd/http-relay-client/client/client.go +++ b/src/go/cmd/http-relay-client/client/client.go @@ -432,7 +432,6 @@ func (c *Client) streamBytes(id string, in io.ReadCloser, out chan<- []byte) { log.Printf("[%s] Got EOF reading from backend", id) } close(out) - in.Close() } // buildResponses collates the bytes from the in stream into HttpResponse objects. @@ -584,8 +583,6 @@ func (c *Client) handleRequest(remote *http.Client, local *http.Client, pbreq *p c.postErrorResponse(remote, id, errorMessage) return } - defer hresp.Body.Close() - // hresp.Body is also closed from streamBytes() if *resp.StatusCode == http.StatusSwitchingProtocols { // A 101 Switching Protocols response means that the request will be @@ -600,6 +597,10 @@ func (c *Client) handleRequest(remote *http.Client, local *http.Client, pbreq *p } // Stream stdin from remote to backend go c.streamToBackend(remote, id, bodyWriter) + } else { + // `streamToBackend` will close `hresp.Body` but it is only called on websocket connections. + // We need to close it here for http connections. + defer hresp.Body.Close() } ctx, respChSpan := trace.StartSpan(ctx, "Building (chunked) response channel")