@marcel91200 / caddy Mirror · read-only / commits / 9be8fb27
Fork 0

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS

Mirrored from https://github.com/Slazee/caddy.git

Synced Nov 11, 04:45 UTC

reverseproxy: propagate TCP half-close on upgraded streams (#8027)

* reverseproxy: propagate TCP half-close on upgraded streams handleUpgradeResponse starts one copy goroutine per direction but returns as soon as the first one reports a result, and returning runs two unconditional deferred closes (the client connection and the backend connection). A client that ends its send direction with a TCP half-close therefore causes the whole tunnel to be torn down: the backend connection is reset mid-conversation and whatever it was about to send is never delivered. Treat a clean EOF as the end of one direction rather than the end of the tunnel. When a copy finishes without error, propagate the half-close to the destination with CloseWrite where the connection supports it, and keep waiting for the other direction so pending bytes can still drain. Real copy errors, the stream timeout and request cancellation/shutdown still tear down immediately. If the destination does not support CloseWrite, the peer cannot be told that the direction ended, so the previous behavior is kept and the tunnel is torn down rather than held open until the stream timeout. This is the same class that was fixed upstream in net/http/httputil (https://go.dev/issue/35892); Caddy has its own upgraded-stream copier, so the standard library fix does not apply here. Fixes #8026 * reverseproxy: follow net/http/httputil shape for the half-close Replace the copyResult struct and the closeWriter/closeWrite helpers with upstream's plain error channel and errCopyDone sentinel, propagating CloseWrite inside the copier, as requested in review. Behaviour is unchanged. The wait is repeated twice inside the existing select because handleUpgradeResponse also selects on the stream timeout and sizes the channel at 2 so both goroutines can exit when it fires (#7418), which net/http/httputil has no equivalent of. * reverseproxy: explain the repeated wait next to it Move the reasoning for why the wait is repeated inside the select, rather than written as upstream's two-line form, into a comment beside the loop so a future reader does not have to find the review discussion. * reverseproxy: add an upgraded-stream half-close integration test Drive a real upgraded stream through the handler and check that closing one direction is propagated to the other side instead of tearing the tunnel down. Mirrors net/http/httputil's TestReverseProxyWebSocketHalfTCP, which covers the same class upstream (https://go.dev/issue/35892): a backend that hijacks and hands back a *net.TCPConn, the handler in front of it, and a client that dials it directly, then the four close-read / close-write combinations. Against the unfixed handler the two half-close cases fail - the surviving direction never delivers its pending bytes - while the two close-read cases pass, so the test isolates exactly the reported behaviour. * reverseproxy: give the half-close test reads a deadline Without one, a regression blocks in ReadFull until the package timeout instead of failing the test, which is the flakiness an end-to-end test of this kind is usually accused of. Ten seconds is far above any real latency here and turns a hang into a clear failure. * reverseproxy: hijack the test backend via http.NewResponseController Matches how the rest of the tree reaches Hijack (responsewriter.go, caddyauth.go, streaming.go itself) and keeps working if the writer is wrapped, where a direct http.Hijacker assertion would not.

Turhan ACAR committed Sep 16, 2026 at 11:10 UTC 9be8fb279444ec633ae2eea4590a180e1143c8f6
2 files changed +351 -16
modules/caddyhttp/reverseproxy/streaming.go
+67 -14
@@ -226,14 +226,42 @@ func (h *Handler) handleUpgradeResponse(logger *zap.Logger, wg *sync.WaitGroup,
226 wg.Add(2)
227 go spc.copyToBackend(errc)
228 go spc.copyFromBackend(errc)
229 - select {
230 - case err := <-errc:
231 - if c := logger.Check(zapcore.DebugLevel, "streaming error"); c != nil {
232 - c.Write(zap.Error(err))
233 - }
234 - case time := <-timeoutc:
235 - if c := logger.Check(zapcore.DebugLevel, "stream timed out"); c != nil {
236 - c.Write(zap.Time("timeout", time))
229 +
230 + // Wait for both directions to finish. A clean EOF in one direction is a
231 + // half-close of that direction only, not the end of the tunnel: the copier
232 + // propagates it to the destination with CloseWrite and reports nil, so the
233 + // other direction is left open and pending bytes can still drain. Anything
234 + // else ends the tunnel: a copy error, a failed CloseWrite, or errCopyDone
235 + // when the destination has no write half to close.
236 + //
237 + // net/http/httputil expresses the same wait as:
238 + //
239 + // err := <-errc
240 + // if err == nil {
241 + // err = <-errc
242 + // }
243 + //
244 + // which cannot be used verbatim here because this handler also selects on the
245 + // stream timeout, so the wait is repeated inside that select instead. Both
246 + // accept the same sequences; the loop only additionally lets the timeout win
247 + // at either wait.
248 + //
249 + // See https://github.com/caddyserver/caddy/issues/8026, and the same class
250 + // fixed upstream in net/http/httputil (https://go.dev/issue/35892).
251 + for range 2 {
252 + select {
253 + case err := <-errc:
254 + if err != nil {
255 + if c := logger.Check(zapcore.DebugLevel, "streaming error"); c != nil {
256 + c.Write(zap.Error(err))
257 + }
258 + return
259 + }
260 + case time := <-timeoutc:
261 + if c := logger.Check(zapcore.DebugLevel, "stream timed out"); c != nil {
262 + c.Write(zap.Time("timeout", time))
263 + }
264 + return
265 }
266 }
267 }
@@ -644,16 +672,41 @@ type switchProtocolCopier struct {
672 bufferSize int
673 }
674
675 +// errCopyDone is reported by a direction that ended at a clean EOF on a
676 +// connection whose write half cannot be closed, so the end of that direction
677 +// could not be propagated to the peer.
678 +var errCopyDone = errors.New("hijacked connection copy complete")
679 +
680 func (c switchProtocolCopier) copyFromBackend(errc chan<- error) {
648 - _, err := io.CopyBuffer(c.user, c.backend, c.buffer())
649 - errc <- err
650 - c.wg.Done()
681 + defer c.wg.Done()
682 + if _, err := io.CopyBuffer(c.user, c.backend, c.buffer()); err != nil {
683 + errc <- err
684 + return
685 + }
686 +
687 + // backend conn has reached EOF so propagate close write to user conn
688 + if wc, ok := c.user.(interface{ CloseWrite() error }); ok {
689 + errc <- wc.CloseWrite()
690 + return
691 + }
692 +
693 + errc <- errCopyDone
694 }
695
696 func (c switchProtocolCopier) copyToBackend(errc chan<- error) {
654 - _, err := io.CopyBuffer(c.backend, c.user, c.buffer())
655 - errc <- err
656 - c.wg.Done()
697 + defer c.wg.Done()
698 + if _, err := io.CopyBuffer(c.backend, c.user, c.buffer()); err != nil {
699 + errc <- err
700 + return
701 + }
702 +
703 + // user conn has reached EOF so propagate close write to backend conn
704 + if wc, ok := c.backend.(interface{ CloseWrite() error }); ok {
705 + errc <- wc.CloseWrite()
706 + return
707 + }
708 +
709 + errc <- errCopyDone
710 }
711
712 func (c switchProtocolCopier) buffer() []byte {
modules/caddyhttp/reverseproxy/streaming_test.go
+284 -2
@@ -1,14 +1,22 @@
1 package reverseproxy
2
3 import (
4 + "bufio"
5 "bytes"
6 + "errors"
7 "io"
8 + "net"
9 + "net/http"
10 "net/http/httptest"
11 + "net/url"
12 + "runtime"
13 "strings"
14 "sync"
15 "testing"
16 + "time"
17
18 "github.com/caddyserver/caddy/v2"
19 + "github.com/caddyserver/caddy/v2/modules/caddyhttp"
20 )
21
22 func TestHandlerCopyResponse(t *testing.T) {
@@ -58,14 +66,91 @@ func TestSwitchProtocolCopierBufferSize(t *testing.T) {
66 go copier.copyToBackend(errc)
67 wg.Wait()
68
61 - if err := <-errc; err != nil {
62 - t.Fatalf("copyToBackend() error = %v", err)
69 + // the destination cannot be half-closed, so a clean copy reports errCopyDone
70 + if err := <-errc; !errors.Is(err, errCopyDone) {
71 + t.Fatalf("copyToBackend() error = %v, want %v", err, errCopyDone)
72 }
73 if got := dst.String(); got != "hello" {
74 t.Fatalf("copied data = %q, want %q", got, "hello")
75 }
76 }
77
78 +// A clean EOF in one direction must be propagated to the destination as a
79 +// write-half close instead of tearing the whole tunnel down.
80 +// See https://github.com/caddyserver/caddy/issues/8026.
81 +func TestSwitchProtocolCopierHalfClose(t *testing.T) {
82 + closeWriteErr := errors.New("no half-close")
83 +
84 + for _, tc := range []struct {
85 + name string
86 + dst io.ReadWriteCloser
87 + wantErr error // nil means the half-close was propagated
88 + }{
89 + {
90 + name: "destination supports CloseWrite",
91 + dst: &closeWriteRecorder{},
92 + wantErr: nil,
93 + },
94 + {
95 + name: "destination does not support CloseWrite",
96 + dst: nopReadWriteCloser{Writer: io.Discard},
97 + wantErr: errCopyDone,
98 + },
99 + {
100 + name: "CloseWrite fails",
101 + dst: &closeWriteRecorder{err: closeWriteErr},
102 + wantErr: closeWriteErr,
103 + },
104 + } {
105 + t.Run(tc.name, func(t *testing.T) {
106 + var wg sync.WaitGroup
107 + errc := make(chan error, 1)
108 + copier := switchProtocolCopier{
109 + user: nopReadWriteCloser{Reader: strings.NewReader("hello")},
110 + backend: tc.dst,
111 + wg: &wg,
112 + }
113 +
114 + wg.Add(1)
115 + go copier.copyToBackend(errc)
116 + wg.Wait()
117 +
118 + if err := <-errc; !errors.Is(err, tc.wantErr) {
119 + t.Fatalf("copyToBackend() error = %v, want %v", err, tc.wantErr)
120 + }
121 + if rec, ok := tc.dst.(*closeWriteRecorder); ok && !rec.called {
122 + t.Fatal("CloseWrite() was not called on a destination that supports it")
123 + }
124 + })
125 + }
126 +}
127 +
128 +// A copy error must be reported as-is and must not be mistaken for a clean
129 +// half-close, so the tunnel is still torn down immediately.
130 +func TestSwitchProtocolCopierErrorIsNotHalfClose(t *testing.T) {
131 + var wg sync.WaitGroup
132 + errc := make(chan error, 1)
133 + wantErr := errors.New("write failed")
134 + dst := &closeWriteRecorder{writeErr: wantErr}
135 +
136 + copier := switchProtocolCopier{
137 + user: nopReadWriteCloser{Reader: strings.NewReader("hello")},
138 + backend: dst,
139 + wg: &wg,
140 + }
141 +
142 + wg.Add(1)
143 + go copier.copyToBackend(errc)
144 + wg.Wait()
145 +
146 + if err := <-errc; !errors.Is(err, wantErr) {
147 + t.Fatalf("error = %v, want %v", err, wantErr)
148 + }
149 + if dst.called {
150 + t.Fatal("CloseWrite() was called after a copy error")
151 + }
152 +}
153 +
154 func TestSwitchProtocolCopierDefaultBufferSize(t *testing.T) {
155 copier := switchProtocolCopier{}
156 buf := copier.buffer()
@@ -80,3 +165,200 @@ type nopReadWriteCloser struct {
165 }
166
167 func (nopReadWriteCloser) Close() error { return nil }
168 +
169 +// closeWriteRecorder is a destination that supports closing only its write
170 +// half, and records whether that happened.
171 +type closeWriteRecorder struct {
172 + called bool
173 + err error
174 + writeErr error
175 +}
176 +
177 +func (c *closeWriteRecorder) Read([]byte) (int, error) { return 0, io.EOF }
178 +
179 +func (c *closeWriteRecorder) Write(p []byte) (int, error) {
180 + if c.writeErr != nil {
181 + return 0, c.writeErr
182 + }
183 + return len(p), nil
184 +}
185 +
186 +func (c *closeWriteRecorder) Close() error { return nil }
187 +
188 +func (c *closeWriteRecorder) CloseWrite() error {
189 + c.called = true
190 + return c.err
191 +}
192 +
193 +// TestHandlerUpgradedStreamHalfClose drives a real upgraded stream through the
194 +// handler and checks that closing one direction is propagated to the other side
195 +// instead of tearing the whole tunnel down.
196 +//
197 +// It mirrors net/http/httputil's TestReverseProxyWebSocketHalfTCP, which covers
198 +// the same class upstream (https://go.dev/issue/35892). Before the fix, the
199 +// "close write" cases failed: the handler returned as soon as the first
200 +// direction reported, and the deferred closes killed the other direction before
201 +// its pending bytes could be delivered.
202 +//
203 +// See https://github.com/caddyserver/caddy/issues/8026.
204 +func TestHandlerUpgradedStreamHalfClose(t *testing.T) {
205 + switch runtime.GOOS {
206 + case "plan9", "js", "wasip1":
207 + t.Skipf("not supported on %s", runtime.GOOS)
208 + }
209 +
210 + // Reads carry a deadline so a regression fails the test promptly instead of
211 + // blocking until the whole package times out.
212 + const readTimeout = 10 * time.Second
213 +
214 + mustRead := func(t *testing.T, conn *net.TCPConn, msg string) {
215 + t.Helper()
216 + if err := conn.SetReadDeadline(time.Now().Add(readTimeout)); err != nil {
217 + t.Fatalf("failed to set read deadline: %v", err)
218 + }
219 + b := make([]byte, len(msg))
220 + if _, err := io.ReadFull(conn, b); err != nil {
221 + t.Fatalf("failed to read: %v", err)
222 + }
223 + if got, want := string(b), msg; got != want {
224 + t.Fatalf("got %#q, want %#q", got, want)
225 + }
226 + }
227 +
228 + mustReadEOF := func(t *testing.T, conn *net.TCPConn) {
229 + t.Helper()
230 + if err := conn.SetReadDeadline(time.Now().Add(readTimeout)); err != nil {
231 + t.Fatalf("failed to set read deadline: %v", err)
232 + }
233 + b := make([]byte, 1)
234 + if _, err := conn.Read(b); !errors.Is(err, io.EOF) {
235 + t.Fatalf("read after peer half-close: got %v, want EOF", err)
236 + }
237 + }
238 +
239 + mustWrite := func(t *testing.T, conn *net.TCPConn, msg string) {
240 + t.Helper()
241 + if _, err := conn.Write([]byte(msg)); err != nil {
242 + t.Fatalf("failed to write: %v", err)
243 + }
244 + }
245 +
246 + mustCloseRead := func(t *testing.T, conn *net.TCPConn) {
247 + t.Helper()
248 + if err := conn.CloseRead(); err != nil {
249 + t.Fatalf("failed to CloseRead: %v", err)
250 + }
251 + }
252 +
253 + mustCloseWrite := func(t *testing.T, conn *net.TCPConn) {
254 + t.Helper()
255 + if err := conn.CloseWrite(); err != nil {
256 + t.Fatalf("failed to CloseWrite: %v", err)
257 + }
258 + }
259 +
260 + tests := map[string]func(t *testing.T, cli, srv *net.TCPConn){
261 + "backend close read": func(t *testing.T, cli, srv *net.TCPConn) {
262 + mustCloseRead(t, srv)
263 + mustWrite(t, srv, "backend sends")
264 + mustRead(t, cli, "backend sends")
265 + },
266 + "backend close write": func(t *testing.T, cli, srv *net.TCPConn) {
267 + mustCloseWrite(t, srv)
268 + mustWrite(t, cli, "client sends")
269 + mustRead(t, srv, "client sends")
270 + mustReadEOF(t, cli)
271 + },
272 + "client close read": func(t *testing.T, cli, srv *net.TCPConn) {
273 + mustCloseRead(t, cli)
274 + mustWrite(t, cli, "client sends")
275 + mustRead(t, srv, "client sends")
276 + },
277 + "client close write": func(t *testing.T, cli, srv *net.TCPConn) {
278 + mustCloseWrite(t, cli)
279 + mustWrite(t, srv, "backend sends")
280 + mustRead(t, cli, "backend sends")
281 + mustReadEOF(t, srv)
282 + },
283 + }
284 +
285 + for name, test := range tests {
286 + t.Run(name, func(t *testing.T) {
287 + srvc := make(chan *net.TCPConn, 1)
288 +
289 + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
290 + conn, _, err := http.NewResponseController(w).Hijack()
291 + if err != nil {
292 + t.Errorf("backend hijack failed: %v", err)
293 + return
294 + }
295 + tcp, ok := conn.(*net.TCPConn)
296 + if !ok {
297 + conn.Close()
298 + t.Errorf("backend conn is %T, want *net.TCPConn", conn)
299 + return
300 + }
301 + if _, err := io.WriteString(tcp, "HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n\r\n"); err != nil {
302 + tcp.Close()
303 + t.Errorf("backend upgrade write failed: %v", err)
304 + return
305 + }
306 + srvc <- tcp
307 + }))
308 + defer backend.Close()
309 +
310 + h := minimalHandler(0, &Upstream{
311 + Host: new(Host),
312 + Dial: backend.Listener.Addr().String(),
313 + })
314 + h.connections = make(map[io.ReadWriteCloser]openConnection)
315 + h.connectionsMu = new(sync.Mutex)
316 +
317 + frontend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
318 + _ = h.ServeHTTP(w, prepareTestRequest(r), caddyhttp.HandlerFunc(func(http.ResponseWriter, *http.Request) error {
319 + return nil
320 + }))
321 + }))
322 + defer frontend.Close()
323 +
324 + frontendURL, err := url.Parse(frontend.URL)
325 + if err != nil {
326 + t.Fatalf("failed to parse frontend URL: %v", err)
327 + }
328 + addr, err := net.ResolveTCPAddr("tcp", frontendURL.Host)
329 + if err != nil {
330 + t.Fatalf("failed to resolve TCP address: %v", err)
331 + }
332 + cli, err := net.DialTCP("tcp", nil, addr)
333 + if err != nil {
334 + t.Fatalf("failed to dial frontend: %v", err)
335 + }
336 + defer cli.Close()
337 +
338 + req, _ := http.NewRequest(http.MethodGet, frontend.URL, nil)
339 + req.Header.Set("Connection", "Upgrade")
340 + req.Header.Set("Upgrade", "websocket")
341 + if err := req.Write(cli); err != nil {
342 + t.Fatalf("failed to write upgrade request: %v", err)
343 + }
344 +
345 + resp, err := http.ReadResponse(bufio.NewReader(cli), &http.Request{Method: http.MethodGet})
346 + if err != nil {
347 + t.Fatalf("failed to read upgrade response: %v", err)
348 + }
349 + if resp.StatusCode != http.StatusSwitchingProtocols {
350 + t.Fatalf("status = %d, want 101", resp.StatusCode)
351 + }
352 +
353 + var srv *net.TCPConn
354 + select {
355 + case srv = <-srvc:
356 + case <-time.After(5 * time.Second):
357 + t.Fatal("timed out waiting for the backend connection")
358 + }
359 + defer srv.Close()
360 +
361 + test(t, cli, srv)
362 + })
363 + }
364 +}