Skip to content

wsflate: handle short compressor EOF blocks - #227

Open
sb123sb123 wants to merge 2 commits into
gobwas:masterfrom
sb123sb123:fix/go127-flate-close
Open

sb123sb123 wants to merge 2 commits into
gobwas:masterfrom
sb123sb123:fix/go127-flate-close

Conversation

@sb123sb123

Copy link
Copy Markdown

Fixes #221

Problem

wsflate.Writer assumes that a compressor's Close leaves the four-byte stored-block tail (00 00 ff ff) in its buffer. Go 1.27's compress/flate emits a short fixed-Huffman EOF block instead, so closing after the required PMCE flush can move buffered bytes to the destination and return a false wsflate: bad compressor error.

Cause

The PMCE sync-flush marker and the compressor's final EOF block are different protocol concerns, but Close checked and forwarded them through the same four-byte tail buffer.

Fix

Track whether the last successful operation was Flush. When closing a flushed writer, still call the compressor's Close method, but discard only its close-time output so the PMCE message remains free of the final block. A subsequent Write clears the state and requires a new flush.

The regression test models the short fixed-Huffman EOF block and verifies that the compressor is closed, no error is reported, and the flushed payload is unchanged.

Tests

  • go test -count=1 -run '^TestWriterCloseAfterFlushWithShortFinalBlock$' ./wsflate (passed with task-local dependency replacements)
  • go test ./... (passed with task-local dependency replacements)
  • go vet ./wsflate (passed)
  • gofmt -d wsflate/writer.go wsflate/writer_test.go (clean)
  • git diff --check (clean)

Limitations

The remote Windows host runs Go 1.26.8, so it could not execute the Go 1.27 compressor directly. The Go module proxy and direct module fetches were unavailable; I used temporary local copies of the two small dependencies for testing and removed them before committing. Full-module vet also reports two pre-existing findings outside this change: wsutil/writer_test.go:135 (reflect.SliceHeader) and example/autobahn/autobahn.go:58 (discarded context.WithTimeout cancel function). The race test was not run because the host has no gcc C compiler.

AI assistance disclosure

This PR was prepared with assistance from OpenAI Codex. Codex performed issue triage, implementation, regression-test authoring, and validation; the final patch and reported results were checked against the repository and command output.

Comment thread wsflate/writer.go Outdated
w.flushed = false
w.err = w.c.Flush()
w.checkTail()
if w.err == nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

w.flushed = w.err == nil ?

@sb123sb123

Copy link
Copy Markdown
Author

Thanks for the review and approval. The Go CI matrix for this head is green. The separate Autobahn workflow fails while building its test image, before the Autobahn suite starts: installing autobahntestsuite==0.8.2 fails in the Docker build because pycparser is missing from the cffi build requirements; the later docker run ws-autobahn then cannot find the image. The PR changes only wsflate code and tests, so this appears isolated to the test image setup. Would you prefer a separate CI fix, or is there a recommended way to handle this check for this PR?

Failing run: https://github.com/gobwas/ws/actions/runs/36542593276

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.

Possible incoming breakage from upstream golang

2 participants