compress/zlib: reading data with known length

When the length of the compressed data is known a priori, and one reads precisely as many bytes from the ReadCloser returned by zlib.NewReader, the zlib checksum isn't read. One needs to attempt to read past the end of the compressed stream to get zlib to read it, which I think this is counterintuitive, and makes it so that one pretty much has to use ioutil.ReadAll to safely read zlibbed segments embedded in a larger stream.

Obligatory play.golang.org link: https://play.golang.org/p/KvqnDU36Q2

A good solution, I think, would be to arrange to have Close read and check the checksum if it hasn't been done yet, as it should be called when one is done reading anyway.

Tested on go1.5 linux/amd64.

[bradfitz]

I think the safest thing we can do here is document this. Changing the behavior of Close to start doing reads and changing the return value from Close seems like an Go 1 compatibility promise change at this point.

Btw, a more convincing play.golang.org snippet might include checking return values, such as: https://play.golang.org/p/dI-lca_rjR ... because notably, the zlib.Reader docs explicitly says it can over-read from the underlying reader, so what you should be testing isn't the number of bytes remaining, but what your desired return values are.

[dsnet]

I don't think having Close do a 4-byte read is a valid solution. The zlib format does not encode the uncompressed size, so there is no way for Close to know that the next 4 bytes is the checksum. This happens to work for you since you know that information, but the Reader implementation does not know that. It only knows to read the checksum when it receives an EOF from the underlying flate.Reader. However, the current implementation of flate.Read always returns (n, nil) if n is positive. Thus, zlib.Reader cannot get an EOF unless an attempt to read past the length of the stream is made.

Thus, I agree with @bradfitz that the best solution is to just document this situation.

On a side note, the documentation about "The implementation buffers input and may read more data than necessary from r." should probably be updated since it is not always true. If the reader passed in satisfies the io.ByteReader interface, it will not read more data than necessary.

If altering Close is not acceptable, could flate.Read be changed to return io.EOF on the "last" read and not on the first read past the end? This should be possible, since the last block in a DEFLATE stream is explicitly marked as such.

As an aside, what is the purpose of the Close method, actually? Looking at the implementation in both compress/zlib and compress/flate, it seems to merely return the stored error (if any), but said error will have already been returned by the call that generated it, which a well-behaved client should've also checked.

[dsnet]

You are correct that the last block in DEFLATE is marked as such, but as I noted above, the current implementation of compress/flate does not make that easily known. We could alter flate to report EOF more agressively, but that would be a more invasive change to flate. It's certainly do-able, but it's also very fragile, because it means that zlib is dependent on more strict behavior than what io.Reader promises.

I'm wondering, you seem to be attempting to read a series of zlib encoded stream back-to-back and you know the uncompressed size. Can you do something similar to the following?

var rd io.Reader // Let's say your data stream is in here
chunks := []int64{13, 1323, 251} // Uncompressed size of each chunk

// Wrap your reader with bufio.Reader to ensure that it satisfies io.ByteReader.
rd = bufio.NewReader(rd)

for _, nchk := range chunks {
    zr, err := zlib.NewReader(rd)
    if err != nil {
        panic(err)
    }

    var buf bytes.Buffer
    n, err := io.CopyN(&buf, zr, nchk+1) // Read one byte past EOF to fully consume stream
    switch {
    case err == nil || n > nchk:
        panic("ZLIB stream is longer than expected")
    case err != io.EOF:
        panic(err)
    case n < nchk:
        panic("ZLIB stream is shorter than expected")
    }

    if err := zr.Close(); err != nil {
        panic(err)
    }
}

P.S. io.CopyN does allocate a buffer on each use. You could use a combination io.LimitedReader and io.CopyBuffer to avoid that allocation if performance matters.

[gopherbot]

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

[bradfitz]

@dsnet, btw, I've made a number of changes over the years to return (n, io.EOF) instead of (n, nil) + (0, io.EOF) more aggressively. Either is valid under the io.Reader contract, but there are a number of performance reasons for returning io.EOF early. I'd be in favor of doing so in compress/flate if it doesn't already.

[dsnet]

@bradfitz, I'd be happy to make that change in the inflate code :)

What are your thoughts on my concern about having zlib be dependent on flate.Read always returning io.EOF aggressively? Making the change for performance changes is one thing, relying on that change for correctness of some feature is another. It seems brittle to me...

[bradfitz]

Define "dependent"? If we want the final zlib.Reader.Read (the (non-zero, io.EOF) read) to return an error if the 4 byte checksum following is bogus, that doesn't seem fragile: it's a dependency is the same repo, and we write tests.

[dsnet]

SGTM, I'll submit CLs in the near future.

@dsnet: Yes, reading back-to-back zlib streams with known uncompressed sizes is my use case. Getting it done isn't a problem - I've just been using ioutil.ReadAll to date - but I raised this issue to see if, sometime in the future, I could maybe get it done with io.ReadFull and some appropriately pre-allocated slices.

[gopherbot]

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

[dsnet]

I thought about this over the weekend and I don't think compress/flate should be changed since this changes some subtle behavior with regard to how flushing works. By convention, a DEFLATE reader stops reading when it encounters an [empty, raw block] as a signal that all data read so far should be flushed to the user.

Consider the following DEFLATE stream:

[non-empty N=10, raw block] [empty, raw block] [final, empty, fixed block]
                                              ^
                              Read must stop here, by flushing semantics;
                              But can't report EOF, since that's in the next block

In this situation, suppose you read exactly 10 bytes (knowing that was the length of the stream), in order for the DEFLATE to know that EOF was encountered, it must read through all empty blocks until it either 1) hits another block with data, or 2) hits an empty block with the final bit set. However, this might break some network protocols that have subtle dependencies on flushing (for example, that the read offset of the underlying reader was left immediately after the distinctive 0x0000ffff marker of empty raw blocks).

Let's just document this situation.

[gopherbot]

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

[gopherbot]

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