From b180ba533214bde385a2fb79f7bc87bedf47c453 Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Mon, 16 Feb 2026 16:29:03 -0800 Subject: [PATCH] Make compression reader/writer Close() idempotent The flateReadWrapper.Read() preemptively calls Close() when it encounters EOF, returning the flate reader to the pool early. When NextReader() is subsequently called, it calls Close() again on the same wrapper. Previously, this second Close() returned io.ErrClosedPipe, which caused spurious errors when compression was enabled (e.g. "discarding reader close error: io: read/write on closed pipe"). Per RFC 6455 and standard Go conventions, closing an already-closed resource should be a no-op. This change makes both flateReadWrapper and flateWriteWrapper Close() methods idempotent by returning nil instead of an error when the underlying reader/writer is already nil. Fixes #859 Signed-off-by: Varun Chawla --- compression.go | 4 ++-- compression_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/compression.go b/compression.go index fe1079ed..4c4b8b0b 100644 --- a/compression.go +++ b/compression.go @@ -108,7 +108,7 @@ func (w *flateWriteWrapper) Write(p []byte) (int, error) { func (w *flateWriteWrapper) Close() error { if w.fw == nil { - return errWriteClosed + return nil } err1 := w.fw.Flush() w.p.Put(w.fw) @@ -143,7 +143,7 @@ func (r *flateReadWrapper) Read(p []byte) (int, error) { func (r *flateReadWrapper) Close() error { if r.fr == nil { - return io.ErrClosedPipe + return nil } err := r.fr.Close() flateReaderPool.Put(r.fr) diff --git a/compression_test.go b/compression_test.go index 00ae42f1..c51f4123 100644 --- a/compression_test.go +++ b/compression_test.go @@ -77,3 +77,54 @@ func TestValidCompressionLevel(t *testing.T) { } } } + +func TestFlateReadWrapperDoubleClose(t *testing.T) { + var buf bytes.Buffer + c := newTestConn(nil, &buf, true) + c.newCompressionWriter = compressNoContextTakeover + c.enableWriteCompression = true + + // Write a compressed message to get valid compressed data. + if err := c.WriteMessage(TextMessage, []byte("hello")); err != nil { + t.Fatal(err) + } + + // Set up a reader connection with decompression. + rc := newTestConn(&buf, io.Discard, false) + rc.newDecompressionReader = decompressNoContextTakeover + + _, r, err := rc.NextReader() + if err != nil { + t.Fatal(err) + } + + // Read all data, which triggers preemptive close on EOF. + if _, err := io.ReadAll(r); err != nil { + t.Fatal(err) + } + + // NextReader calls Close() again on the reader. This must not return an + // error because Close() should be idempotent per the fix for #859. + _, _, err = rc.NextReader() + // NextReader blocks waiting for the next frame. Since there's no more data, + // we expect an EOF-related error, not a close error. + if err != nil && err.Error() == "io: read/write on closed pipe" { + t.Fatal("Close() returned error on second call, should be idempotent") + } +} + +func TestFlateWriteWrapperDoubleClose(t *testing.T) { + ww := &flateWriteWrapper{} + // Close on an already-nil writer should return nil, not an error. + if err := ww.Close(); err != nil { + t.Fatalf("expected nil error on double close of flateWriteWrapper, got: %v", err) + } +} + +func TestFlateReadWrapperCloseIdempotent(t *testing.T) { + rw := &flateReadWrapper{} + // Close on an already-nil reader should return nil, not an error. + if err := rw.Close(); err != nil { + t.Fatalf("expected nil error on double close of flateReadWrapper, got: %v", err) + } +}