Reader should not choke on underlying reader errors

[dsnet]

Using 677f6a5.

A large number of io.Reader errors are transient and non-fatal. However, lz4.Reader currently remembers all underlying reader errors and fails to make progress once one has been encountered.

Example program:

var (
	lz4Header = []byte("\x04\"M\x18`@\x82")
	lz4Block1 = []byte("\v\x00\x00\x00\xa0fizz buzz\n")
	lz4Block2 = []byte("\r\x00\x00\x00\xc0foo bar baz\n")
	lz4Footer = []byte("\x00\x00\x00\x00")
)

bb := new(bytes.Buffer)
zr := lz4.NewReader(bb)

bb.Write(lz4Header)
bb.Write(lz4Block1)
pl(io.Copy(os.Stdout, zr)) // should print "fizz buzz"

bb.Write(lz4Block2)
bb.Write(lz4Footer)
pl(io.Copy(os.Stdout, zr)) // should print "foo bar baz"

This currently prints:

fizz buzz
10 <nil>
0 <nil>

I expect it to print:

fizz buzz
10 <nil>
foo bar baz
12 <nil>

The problem is that lz4.Reader see io.EOF and never tries to read from the underlying io.Reader again. However, io.Reader is allowed to report io.EOF at some point, but then stop doing so in the future. It's also allowed to do that for other errors (e.g., a transient network timeout).

[pierrec]

I agree that your example is indeed a bug and it will be fixed. However, I don't see in the io.Reader doc where io.EOF is allowed to be a transient one, or on how to handle transient errors?

[greatroar]

In my experience, when transient errors are possible, you wrap the Reader to deal with them rather than encumbering every consumer with the problem. How would LZ4 know which errors are transient? Specifically in the case of EOF, when a Reader returns n>0, EOF, it is then required to return EOF again on the next call (and I've always taken that to imply it must keep returning EOF, but I admit that's not in the io.Reader docs).

[pierrec]

@dsnet I think I misunderstood your example: it is not that the Reader discards received data along an EOF, it is that it discards data after one. In which case I agree with @greatroar that this behaviour is not the responsability of the LZ4 reader.

[greatroar]

On second thought, maybe I was wrong and it would be better to pass received errors through and not remember them, so the caller can handle them by retrying.

Also, I would actually expect the LZ4 reader to return something other than EOF if it encountered it before the footer. Now, io.Copy pretends that it read a complete file.

Perhaps Read should...

• return EOF only when it has successfully read the footer,
• return io.ErrUnexpectedEOF when it encounters EOF earlier,
• pass all other errors from the underlying Reader through (maybe wrapped),
• remember only lz4 format errors.

Does that sound reasonable? A remaining question is whether EOF in the middle of a stream is a format error to be remembered or an underlying Reader error to be handled by the caller :)

[dsnet]

@greatroar, what you described in your last comment is what I would expect as the behavior.

A remaining question is whether EOF in the middle of a stream is a format error to be remembered or an underlying Reader error to be handled by the caller :)

I expect that to be simply changed into an io.ErrUnexpectedEOF and passed up the stack, but not remembered.

As a general rule-of-thumb, regardless of what errors are returned by the underlying reader, so long as the caller knows that they are temporary, we should be able keep retry calling lz4.Reader.Read and eventually decompress the entire stream (without any data loss or corruption).

That is, the follow should always work:

b := make([]byte, ...)
zr := lz4.NewReader(r)
for {
    n, err := zr.Read(b)
    if n > 0 {
       ... = b[:n] // handle n bytes of uncompressed output
    }
    switch {
    case err == io.EOF:
        return nil
    case isTemporaryError(err): // let user code decide whether this is a temporary problem
        continue
    case err != nil:
        return err
    }
}

lz4.Reader should not make any attempt at deciding whether errors returned by the underlying reader are fatal or temporary. Leave that to the user of the lz4 package. The only fatal errors are lz4 format errors, where no amount of retrying will ever make progress.

As an implementation note, lz4.Reader may not even need to remember format errors. So long as the state machine is resumable, it will be able to re-compute the same format error when it tries to parse the next sequence of bytes that contains a format error. It may cause a little bit of extra work each time to discover what is essentially the same error, but might make the implementation simpler.

pass all other errors from the underlying Reader through (maybe wrapped),

Wrapping is fine so long as it's compatible with errors.Unwrap.

[klauspost]

@dsnet I disagree on your assumption. A Reader that has returned any error is not in a usable state, and should IMO not be used any more. I don't think it is unreasonable for this package to assume an un-broken stream of input.

The stdlib inflate has stateful errors. The official snappy package also has stateful errors and I am pretty sure you can find more examples.

As pointed out, there is nothing preventing you from wrapping the input Reader in retry code, that can do whatever it wants on what is transient errors in your case.