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

proposal: compress/flate: deprecate ReadError and WriteError #12724

Closed
nigeltao opened this issue Sep 23, 2015 · 6 comments
Closed

proposal: compress/flate: deprecate ReadError and WriteError #12724

nigeltao opened this issue Sep 23, 2015 · 6 comments

Comments

@nigeltao
Copy link
Contributor

Their doc comments give a hint towards their original intent, written Go's early days:


// A ReadError reports an error encountered while reading input.
type ReadError struct { etc }

// A WriteError reports an error encountered while writing output.
type WriteError struct { etc }


The WriteError type is simply unused by the package:

$ grep WriteError src/compress/flate/*.go
src/compress/flate/inflate.go:// A WriteError reports an error encountered while writing output.
src/compress/flate/inflate.go:type WriteError struct {
src/compress/flate/inflate.go:func (e *WriteError) Error() string {

The ReadError type is sometimes used by the package, but it's not returned consistently enough for users of that package to rely on it. For example, as reported on issue #11856, this program (https://play.golang.org/p/DrL0flbZFg):


package main

import "bytes"
import "io/ioutil"
import "fmt"
import "compress/flate"

func printReadError(lvl int) {
b := bytes.NewBuffer(nil)
wr, _ := flate.NewWriter(b, lvl)
wr.Write([]byte("the quick brown fox jumped over the lazy dog"))
wr.Close()
bb := b.Bytes()
rd := flate.NewReader(bytes.NewReader(bb[:len(bb)/2]))
_, err := ioutil.ReadAll(rd)
fmt.Println(err)
}

func main() {
printReadError(flate.NoCompression) // flate.ReadError used
printReadError(flate.DefaultCompression) // flate.ReadError not used
}


Prints:
flate: read error at offset 27: unexpected EOF
unexpected EOF

Thus, it is highly unlikely that existing code was depending on Read always or even sometimes returning ReadError. On the other hand, the possibility of returning a ReadError complicates test code like https://go-review.googlesource.com/#/c/14832/3/src/compress/gzip/gunzip_test.go which says:

_, err = io.Copy(ioutil.Discard, r)
if ferr, ok := err.(*flate.ReadError); ok {
err = ferr.Err
}

The proposal is to change the doc comments (but not the exported fields and methods) in inflate.go to be:


// ReadError is deprecated.
type ReadError struct { etc }

// WriteError is deprecated.
type WriteError struct { etc }


and to remove any other ReadError references in that package, so that the two occurences of:
f.err = &ReadError{f.roffset, err}
would become:
f.err = err

There are no references to flate.ReadError or flate.WriteError in the standard library or under golang.org/x, other than golang.org/x/tools/imports/zstdlib.go which is an autogenerated file.

@bradfitz
Copy link
Contributor

SGTM

I'd go further than "is deprecated", which implies it works but should be phased out. I'd say it's unused and only retained for API compatibility or something.

@dsnet
Copy link
Member

dsnet commented Sep 23, 2015

Thanks for writing up this formal proposal, I appreciate it.

@nightlyone
Copy link
Contributor

Sad to see these go instead of being used more consistently, but I agree that they are just baggage as currently implemented.

The offset information of where in a large stream I have an error might be useful in recovery situations.

@robpike
Copy link
Contributor

robpike commented Sep 23, 2015

SGTM

@dsnet
Copy link
Member

dsnet commented Sep 24, 2015

@nightlyone I should note that that this proposal does not remove use of CorruptInputError which still contains information about the offset. Thus, you can still obtain the offset that a compression error occurred.

@nightlyone
Copy link
Contributor

@dsnet thanks for pointing this out, I totally overlooked this. SGTM without any regrets then.

@adg adg added Proposal and removed Proposal labels Sep 25, 2015
@golang golang locked and limited conversation to collaborators Oct 9, 2016
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

7 participants