Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

segiddins
Copy link
Contributor

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 segiddins force-pushed the segiddins/fix-eof-when-data-falls-on-read-boundary branch 3 times, most recently from 65fb579 to 6973398 Compare December 22, 2023 17:23
@segiddins
Copy link
Contributor Author

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

@segiddins
Copy link
Contributor Author

Curious, this passes for me on truffleruby-dev locally

@segiddins
Copy link
Contributor Author

@eregon or @hsbt, are you please able to retry the failing job? I was unable to reproduce it locally (same as in #61)

@eregon
Copy link
Member

eregon commented Jan 13, 2024

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

@eregon
Copy link
Member

eregon commented Jan 13, 2024

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:

zlib/ext/zlib/zlib.c

Lines 2986 to 2999 in 2561e12

const unsigned char *ss, *sp, *se;
unsigned char *ds, *dp, *de;
VALUE cbuf = rb_enc_str_new(0, GZFILE_CBUF_CAPA, gz->enc);
ss = sp = (const unsigned char*)RSTRING_PTR(gz->z.buf);
se = sp + ZSTREAM_BUF_FILLED(&gz->z);
ds = dp = (unsigned char *)RSTRING_PTR(cbuf);
de = (unsigned char *)ds + GZFILE_CBUF_CAPA;
(void)rb_econv_convert(gz->ec, &sp, se, &dp, de, ECONV_PARTIAL_INPUT|ECONV_AFTER_OUTPUT);
rb_econv_check_error(gz->ec);
dst = zstream_shift_buffer(&gz->z, sp - ss);
gzfile_calc_crc(gz, dst);
rb_str_resize(cbuf, dp - ds);
return cbuf;

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
Copy link
Member

eregon commented Jan 13, 2024

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 #75, that failing job should be ignored until then.

eregon added a commit to eregon/zlib that referenced this pull request Jan 13, 2024
@eregon
Copy link
Member

eregon commented Jan 15, 2024

CI fixed in #76 so if you rebase it should be all green.

…he stream is not yet finished

See code comments for details

Fixes ruby#56
@segiddins segiddins force-pushed the segiddins/fix-eof-when-data-falls-on-read-boundary branch from 6973398 to 614708f Compare January 16, 2024 19:57
@segiddins
Copy link
Contributor Author

@eregon rebased!

@segiddins
Copy link
Contributor Author

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

@eregon eregon requested review from nobu and nurse February 21, 2024 11:04
Copy link
Member

@nobu nobu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, since sub_test_case is not available in ruby core's test/unit, this test cases won't run after merging into ruby/ruby.

And the most part of the bock seems dispatching by method, it feels better to split for each methods.

This time, I'll merge #72, which has virtually the same fix.
Of course, test enhancements are welcome.

test/zlib/test_zlib.rb Outdated Show resolved Hide resolved
Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>
@segiddins segiddins closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Zlib::GzipReader#readpartial until eof? causes EOFError
3 participants