Hi! Thanks for maintaining zstd-ruby.
I wanted to stream a large .tar.zst through Gem::Package::TarReader and tried Zstd::StreamReader (added in #59, still marked experimental). I ran into a few things that I think block that use case, and I'd like to check whether you'd be open to changing them before I send a patch.
Environment: zstd-ruby 2.0.6, also reproduced on main @ d0adf3f; Ruby 3.3.11.
1. length is interpreted as compressed bytes
data = @io.read(length) # length COMPRESSED bytes
@stream.decompress(data) # returns an unrelated number of DECOMPRESSED bytes
So the size of the return value isn't related to length, and it varies with the compression ratio rather than with what the caller asked for:
tiny = Zstd.compress("hello world\n" * 20_000) # 240_000 bytes -> 44 compressed
Zstd::StreamReader.new(StringIO.new(tiny)).read(512).bytesize
# => 240000
big = Zstd.compress(ndjson) # ~840 KB -> ~20 KB compressed
r = Zstd::StreamReader.new(StringIO.new(big))
4.times.map { r.read(512).bytesize }
# => [0, 0, 0, 0] (zstd is still filling an internal block)
The existing spec captures this too — read(10) currently returns 'a', then 'bcdef'.
Worth noting it's data-dependent: with incompressible input the compressed and decompressed sizes are close, so read looks like it works. It only misbehaves on compressible data, which is presumably why it slipped through.
2. EOF raises instead of returning nil
raise StandardError, "EOF" if @io.eof?
IO#read returns nil at EOF, so generic consumers don't expect an exception here. StandardError is also broad enough that a caller rescuing it to detect EOF would swallow a genuine decompression failure (the C ext raises RuntimeError).
3. close raises NoMethodError
def close
@io.write(@stream.finish)
@io.close
end
StreamingDecompress has only [:decompress, :decompress_with_pos] — no #finish — and @io is open for reading. This looks like it was copied from StreamWriter#close.
Would you accept a fix?
I have a patch that buffers decompressed output so length means decompressed bytes, returns nil at EOF, adds eof?, and fixes close. Full spec suite passes (88 examples).
It does change read's behaviour, so it's breaking for anyone relying on the current semantics — the existing spec's expectations change. Given the class is marked experimental and close has never worked, I'd guess actual usage is small, but it's your call. If you'd rather not break it, I'm equally happy to add a separate IO-conformant class and leave StreamReader alone.
Happy to open the PR either way — just let me know which you prefer.
Hi! Thanks for maintaining zstd-ruby.
I wanted to stream a large
.tar.zstthroughGem::Package::TarReaderand triedZstd::StreamReader(added in #59, still marked experimental). I ran into a few things that I think block that use case, and I'd like to check whether you'd be open to changing them before I send a patch.Environment: zstd-ruby 2.0.6, also reproduced on
main@d0adf3f; Ruby 3.3.11.1.
lengthis interpreted as compressed bytesSo the size of the return value isn't related to
length, and it varies with the compression ratio rather than with what the caller asked for:The existing spec captures this too —
read(10)currently returns'a', then'bcdef'.Worth noting it's data-dependent: with incompressible input the compressed and decompressed sizes are close, so
readlooks like it works. It only misbehaves on compressible data, which is presumably why it slipped through.2. EOF raises instead of returning
nilIO#readreturnsnilat EOF, so generic consumers don't expect an exception here.StandardErroris also broad enough that a caller rescuing it to detect EOF would swallow a genuine decompression failure (the C ext raisesRuntimeError).3.
closeraisesNoMethodErrorStreamingDecompresshas only[:decompress, :decompress_with_pos]— no#finish— and@iois open for reading. This looks like it was copied fromStreamWriter#close.Would you accept a fix?
I have a patch that buffers decompressed output so
lengthmeans decompressed bytes, returnsnilat EOF, addseof?, and fixesclose. Full spec suite passes (88 examples).It does change
read's behaviour, so it's breaking for anyone relying on the current semantics — the existing spec's expectations change. Given the class is marked experimental andclosehas never worked, I'd guess actual usage is small, but it's your call. If you'd rather not break it, I'm equally happy to add a separate IO-conformant class and leaveStreamReaderalone.Happy to open the PR either way — just let me know which you prefer.