Skip to content

Commit

Permalink
Fix trying to close nil connection (#233)
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
koonpeng authored Oct 5, 2023
1 parent c6593f7 commit 9ef8886
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions src/go/cmd/http-relay-client/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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")
Expand Down

0 comments on commit 9ef8886

Please sign in to comment.