search

ruby / zlib

Ruby interface for the zlib compression/decompression library
Other
50 stars 35 forks source link

Fix eof? to return true when there is no more data to read #73

Closed segiddins closed 5 months ago

segiddins commented 10 months ago

Even if the stream is not yet finished

Similar to Socket/Pipe, we need to check if there is any actual data left, as it is possible that the stream is not finished, but there is no data that would be returned upon reading, so we want to report eof? in that case

Fixes #56

Tests comprehensively cover the corner cases of the deflate block boundary falling around multiples of 2048 bytes, which is the chunk size the input is read in.

segiddins commented 10 months ago

Fixed on ruby 2.5, took me a bit to get it installed on my machine

segiddins commented 10 months ago

Curious, this passes for me on truffleruby-dev locally

segiddins commented 9 months ago

@eregon or @hsbt, are you please able to retry the failing job? I was unable to reproduce it locally (same as in https://github.com/ruby/zlib/pull/61)

eregon commented 9 months ago

I restarted it. BTW we should use fail-fast: false so other jobs still run

eregon commented 9 months ago

Weird, it passed on ubuntu: https://github.com/ruby/zlib/actions/runs/7302371715/job/20452572723?pr=73 But failed on macOS: https://github.com/ruby/zlib/actions/runs/7302371715/job/20452572818?pr=73 Because rb_econv_check_error is not defined on TruffleRuby.

Maybe macOS tries to resolve symbols more eagerly than Linux? For instance if it resolves all symbols on method entry vs when the symbol would actually be needed (right before calling/reading it). Just a guess, need to verify that. I think we could workaround this case by moving this code to a different function: https://github.com/ruby/zlib/blob/2561e122aca1241f95aa8a337938fc00f13db14a/ext/zlib/zlib.c#L2986-L2999 I'll try that.

For this PR it's enough if truffleruby-head / ubuntu-latest passes, the macOS failure is unrelated I'd think (but would be good to double-check if fails the same way on master with truffleruby-head on macOS).

eregon commented 9 months ago

Yes it fails the same way on master: https://github.com/ruby/zlib/actions/runs/7512213347/job/20452657764?pr=75 I'll try to fix truffleruby-head / macis-latest in https://github.com/ruby/zlib/pull/75, that failing job should be ignored until then.

eregon commented 9 months ago

CI fixed in https://github.com/ruby/zlib/pull/76 so if you rebase it should be all green.

segiddins commented 9 months ago

@eregon rebased!

segiddins commented 8 months ago

@nobu @nurse I would greatly appreciate a look at this change 🙇🏻