Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compress/zlib: Writer appears to ignore underlying writer errors at times. #16749

Closed
superfell opened this issue Aug 16, 2016 · 2 comments
Closed

Comments

@superfell
Copy link

superfell commented Aug 16, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.7 darwin/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="darwin"
    GOOS="darwin"
    GOPATH="/Users/simon.fell/go"
    GORACE=""
    GOROOT="/usr/local/go"
    GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
    CC="clang"
    GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -gno-record-gcc-switches -fno-common"
    CXX="clang++"
    CGO_ENABLED="1"
  3. What did you do?
    It seems like there's a condition where the zlib Writer doesn't notice that the Writer its writing to returned an error, the test below fails for me on go 1.7 on OSX, passes on go 1.6 linux and go 1.6.3 on osx. The number of writes to zlib seems to be significant in reproducing the error.

The test has a writer that returns an error for the 4th write to the writer, however neither the call the zlib.Writer.Write or to zlib.Writer.Close surface the error.

When running the test, it'll fail with
--- FAIL: Test_WriteErrorCheck (0.00s)
compress_test.go:54: Write count=1, len=2
compress_test.go:54: Write count=2, len=240
compress_test.go:54: Write count=3, len=240
compress_test.go:54: Write count=4, len=240
compress_test.go:54: Write count=5, len=240
compress_test.go:54: Write count=6, len=240
compress_test.go:54: Write count=7, len=240
compress_test.go:54: Write count=8, len=240
compress_test.go:54: Write count=9, len=240
compress_test.go:54: Write count=10, len=240
compress_test.go:54: Write count=11, len=240
compress_test.go:54: Write count=12, len=240
compress_test.go:54: Write count=13, len=240
compress_test.go:54: Write count=14, len=240
compress_test.go:54: Write count=15, len=240
compress_test.go:54: Write count=16, len=240
compress_test.go:54: Write count=17, len=149
compress_test.go:54: Write count=18, len=4
compress_test.go:54: Write count=19, len=4
compress_test.go:38: Underlying writer returned error, that wasn't surfaced to zlib caller [total num writes=2000]
compress_test.go:54: Write count=1, len=2
compress_test.go:54: Write count=2, len=240
compress_test.go:54: Write count=3, len=240
compress_test.go:54: Write count=4, len=240
compress_test.go:54: Write count=5, len=240
compress_test.go:54: Write count=6, len=240
compress_test.go:54: Write count=7, len=240
compress_test.go:54: Write count=8, len=134
compress_test.go:54: Write count=9, len=4
compress_test.go:54: Write count=10, len=4
compress_test.go:38: Underlying writer returned error, that wasn't surfaced to zlib caller [total num writes=2000]
FAIL
FAIL _/Users/simon.fell/code/zlibtest 0.008s

I thought this may of been related to the compression algo for zlib.BestSpeed, but also seem same issue with DefaultCompression as well.

  1. What did you expect to see?
    Expect to see the test pass [as it does on go 1.6]
  2. What did you see instead?
    test fails, no error is surfaces from the zlib Writer.

package main

import (
"compress/zlib"
crand "crypto/rand"
"errors"
"testing"
)

var errWriteFailed = errors.New("I/O Error")

func Test_WriteErrorCheck(t *testing.T) {
src := make([]byte, 2000)
crand.Read(src)
writes := make([][]byte, 2000)
for i := range writes {
writes[i] = src[i/2 : i/2+10]
}
checkWriteErrorSurfaced(t, zlib.BestSpeed, writes)
checkWriteErrorSurfaced(t, zlib.DefaultCompression, writes)
}

func checkWriteErrorSurfaced(t *testing.T, compLevel int, writes [][]byte) bool {
w := &errorWriter{t: t}
c, err := zlib.NewWriterLevel(w, compLevel)
if err != nil {
t.Fatalf("Unable to construct zlib.Writer: %v", err)
}
var errw error
for _, w := range writes {
_, errw = c.Write(w)
if errw != nil {
break
}
}
errc := c.Close()
if errc == nil && errw == nil {
t.Errorf("Underlying writer returned error, that wasn't surfaced to zlib caller [total num writes=%d]", len(writes))
return false
}
t.Logf("%v", errw)
t.Logf("%v", errc)
return true
}

// errorWriter is an io.Writer that returns an error for the 4th write
type errorWriter struct {
t *testing.T
writeCount int
}

func (w *errorWriter) Write(d []byte) (int, error) {
w.writeCount++
w.t.Logf("Write count=%d, len=%d", w.writeCount, len(d))
if w.writeCount == 4 {
return 0, errWriteFailed
}
return len(d), nil
}

@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 16, 2016
@bradfitz bradfitz changed the title zlib.Writer appears to ignore underlying writer errors at times. compress/zlib: Writer appears to ignore underlying writer errors at times. Aug 16, 2016
@dsnet
Copy link
Member

dsnet commented Aug 16, 2016

It's a regression from 53984e5. I'll send out a fix soon.

This is probably go1.7.1 worthy.

@dsnet dsnet removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 16, 2016
@bradfitz bradfitz added this to the Go1.7.1 milestone Aug 16, 2016
@gopherbot
Copy link

CL https://golang.org/cl/27200 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 7, 2016
…ersistent

For persistent error handling, the methods of huffmanBitWriter have to be
consistent about how they check errors. It must either consistently
check error *before* every operation OR immediately *after* every
operation. Since most of the current logic uses the previous approach,
we apply the same style of error checking to writeBits and all calls
to Write such that they only operate if w.err is already nil going
into them.

The error handling approach is brittle and easily broken by future commits to
the code. In the near future, we should switch the logic to use panic at the
lowest levels and a recover at the edge of the public API to ensure
that errors are always persistent.

Fixes #16749

Change-Id: Ie1d83e4ed8842f6911a31e23311cd3cbf38abe8c
Reviewed-on: https://go-review.googlesource.com/27200
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/28634
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Aug 20, 2017
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants