Skip to content

StreamReader doesn't follow Ruby's IO conventions #149

Description

@sribalakumar

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions