compress/flate: Inconsistent error handling

[dsnet]

The package defines a ReadError which "reports an error encountered while reading input" and a WriteError which "reports an error encountered while writing output". However, a quick peruse through the package code shows that WriteError is never used. Secondly, its use of ReadError is inconsistent.

There are 3 instances where the Reader is used in inflate.go: L635, L677, and L699. The call to ReadByte lacks a the wrapper for ReadError. Interestingly, the bulk of DEFLATE compression will be done through the ReadByte call, thus, usage of ReadError seems to only occur when decoding a raw block.

Should use of ReadError and WriteError be removed and marked as deprecated? Or should the library properly handle both error types? Other packages don't seem to wrap IO errors with package specific errors.

[dsnet]

I should also note that usage of InternalError is also inconsistent. The error seems to indicate error in the implementation of flate and should never be triggered (not even with incorrect input) assuming correct implementation. However, most times, the library defaults to panic (used 17 times), and returning an InternalError (used 3 times).

[bradfitz]

/cc @mdempsky @nigeltao

[dsnet]

Inconsistent use of ReadError has existed since the first commit of inflate.go. You can see that the error returned by ReadByte is never wrapped.

I vote that compress/flate stop wrapping IO errors with ReadError and just return the error ad-verbatim since this seems to be the behavior of most other libraries (not to mention that it was rare for the ReadError wrapping to occur in the first place). In the documentation, it should note that ReadError and WriteError are deprecated (#10909) and no longer used.

I also vote that we remove use of InternalError and switch to panic if we can prove that the 3 uses of it really can't happen.

[nigeltao]

Yeah, this was one of the very early Go pacakges, and in hindsight, we'd do it differently these days.

We can't just delete these types, due to backwards compatibility. We could deprecate and stop returning them, I suppose, but I'm not convinced that it's worth changing anything. The benefits seem small, and there's always a risk of breaking someone.

[dsnet]

I don't feel strongly that this be addressed for go1, but it might be worth cleaning up for go2.

I was working on something to detect truncated DEFLATE streams and was expecting to get an ErrUnexpectedEOF wrapped in a ReadError, but was surprised to find out that the library returned the unwrapped ErrUnexpectedEOF most of the time (see: https://play.golang.org/p/DrL0flbZFg). Currently, I have logic that checks for both forms of errors.

We could probably avoid usage of ReadError without breaking anyone since anyone who wanted to handle IO errors would need to have checked for both forms anyways.

[gopherbot]

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

[nigeltao]

I filed a formal proposal: #12724