Repository navigation
Return short-write errors from frame writers - #232
Conversation
| "testing" | ||
| ) | ||
|
|
||
| type writeFunc func([]byte) (int, error) |
There was a problem hiding this comment.
| type writeFunc func([]byte) (int, error) | |
| type writerFunc func([]byte) (int, error) |
There was a problem hiding this comment.
Renamed the helper and its receiver to writerFunc in c4c3f25. The focused short-write tests and full race/coverage suite pass.
| } | ||
| return len(p), nil | ||
| }) | ||
| if err := WriteFrame(w, NewTextFrame([]byte("hello"))); err != test.want { |
There was a problem hiding this comment.
I think here (and in the other test) we should do errors.Is() check to be forward compatible with WriteFrame() returning wrapped errors.
Same applies to the other test.
There was a problem hiding this comment.
Both tests now use errors.Is in c4c3f25. The focused regressions and go test -race -shuffle=on -cover ./... pass on Go 1.26.10.
| {"header_short", 1, nil, io.ErrShortWrite}, | ||
| {"header_error", 1, failure, failure}, | ||
| {"payload_short", 2, nil, io.ErrShortWrite}, | ||
| {"payload_error", 2, failure, failure}, |
There was a problem hiding this comment.
Can we have all structs being initialised like:
{
name: "...",
call: 1,
err: ...,
...
},
{
name: "...",
...
}This makes tests more readable and consistent with the current repo. Also, maybe call the call field something more precise, like "retErrAt" or "failAfter", so err would become failWith or failErr, while want can stay or be exp?
Same applies to the other test.
There was a problem hiding this comment.
Expanded both tables into keyed multiline entries and renamed call/err to retErrAt/failWith in c4c3f25. The full race/coverage suite, build and go vet . pass. go vet ./... still reports two unrelated warnings, both reproduced on the original PR head; the description now distinguishes those results. Autobahn was not run locally.
| WriteHeader, | ||
| func(w io.Writer, h Header) error { return WriteHeaderBuffer(w, h, make([]byte, MaxHeaderSize)) }, | ||
| } { | ||
| if err := write(w, Header{Fin: true, OpCode: OpText, Length: 5}); err != test.want { |
There was a problem hiding this comment.
| if err := write(w, Header{Fin: true, OpCode: OpText, Length: 5}); err != test.want { | |
| h := Header{ | |
| Fin: true, | |
| OpCode: OpText, | |
| Length: 5, | |
| } | |
| if err := write(w, h); err != test.want { |
There was a problem hiding this comment.
Extracted the multiline Header literal as suggested in c4c3f25. The focused header/frame regressions and full race/coverage suite pass.
|
Hey thanks for this work! Just dropped a few comments. |
|
The Go 1.27.3 CI failures reproduce on clean master |
Fixes #150.
Frame writes currently report success when an
io.Writeraccepts only part of the header or payload without returning an error. Returnio.ErrShortWritein those cases, includingWriteHeaderBuffer, and stop before sending the payload if its header was truncated. Explicit writer errors are preserved.Regression fixtures fail on master for short headers and payloads.
go test -race -shuffle=on -cover ./...,go vet .,go build ./...,go mod verify, gofmt andgit diff --checkpass on Go 1.26.10. Full-packagego vet ./...reports two existing warnings (discarded timeout cancellation inexample/autobahn/autobahn.go:58andreflect.SliceHeaderinwsutil/writer_test.go:135); both reproduce unchanged on the original PR head. Autobahn was not run locally.CI update (October 25): the Go 1.27.3 jobs fail with
wsflate: bad compressor: unexpected stream tail: 0xffff0300 vs 0x0000ffff. The same three compression tests fail on clean masterfa8e8b8and this head; the affected compression code is unchanged. This is tracked in #221 and the separate #227. The oldstable CI jobs on Ubuntu and macOS pass. The Autobahn workflow stops in its Go tests before running the Autobahn suite.