zstd: read blocking when unable to fill buffer

[vcabbage]

I ran into an scenario where my application would block when a zstd.Reader was wrapped with a bufio.Reader. When there was data available but less than the 4KB buffer that bufio.Reader uses zstd.Reader.Read appears to block waiting for more.

I can work around this easily by not using a bufio.Reader or passing the buio.Reader to zstd.NewReader, but I was wondering if my interpretation of the code sounds right to you and whether you intended this behavior.

io.Reader docs say "Read conventionally returns what is available instead of waiting for more" so it was slightly surprising but it's not a requirement so I wouldn't call this a bug.

Thank you for all the work you've put into these compression libraries, they're really great!

[klauspost]

When decoding data needs to be available in blocks. This means it is impossible for the decoder to progress until an entire block is available.

To force all data to be output, you can use the Flush function when encoding to force a block break at a specific point.

[vcabbage]

Thanks for the quick response. I am using Flush when writing data.

[klauspost]

ok. I don't remember there being a buffer in the decoder. I can easily have forgotten it, though.

[vcabbage]

I may be misreading, but this section appears to continue to try to read blocks until p is filled or an error is encountered.

compress/zstd/decoder.go

Lines 112 to 129 in 36d74a0

	for {
	if len(d.current.b) > 0 {
	filled := copy(p, d.current.b)
	p = p[filled:]
	d.current.b = d.current.b[filled:]
	n += filled
	}
	if len(p) == 0 {
	break
	}
	if len(d.current.b) == 0 {
	// We have an error and no more data
	if d.current.err != nil {
	break
	}
	d.nextBlock()
	}
	}

My applications stack trace shows it blocking on

compress/zstd/decoder.go

Line 344 in 36d74a0

d.current.decodeOutput = <-d.current.output

So I don't confuse things, only my code is using a bufio.Reader.

[klauspost]

Ok, I see. Yeah - we don't know if d.nextBlock() will block.

Usually it will not. It should be fairly trivial to add a default select here that just whether data is available. Maybe change the signature to func (d *Decoder) nextBlock(block bool) bool

Calls from WriteTo should be blocking. As should calls from Read when no data has been put into the buffer IMO.

[klauspost]

Fixed in master. Released as v1.8.6

[vcabbage]

Thank you!

FWIW, I agree with:

Calls from WriteTo should be blocking. As should calls from Read when no data has been put into the buffer IMO.

The io.Reader docs also discourage returning 0, nil unless the slice passed in has a length of 0.