Repository navigation
Conversation
Fixes tqdm#1847. posix_pipe searched only the fresh chunk from each read: i = tmp.find(delim) When the delimiter is multi-byte and --buf_size is smaller than it, the delimiter's bytes land in two consecutive reads, so neither chunk contains it. The saved prefix is then appended without ever being searched, and the delimiter is missed. Searching buf + tmp finds it, and because buf is non-empty in exactly the case that matters, the indices have to refer to the concatenation: combined = buf + tmp i = combined.find(delim) if i < 0: buf = combined break fp_write(combined[:i + len_delim]) callback(1 if callback_len else combined[:i]) buf = b'' tmp = combined[i + len_delim:] Note that the record passed to the callback must also come from combined, and the leftover chunk is combined[i + len_delim:]. Using tmp[i + len_delim:] would silently drop the bytes still sitting in buf, which is what a partial fix that only changes the find() does. Verified against a 2-byte delimiter at --buf_size 1, 2, 3, 4, 5, 7 and 64: before: 2it instead of 3it, and --update died with SyntaxError: invalid syntax, because the first delimiter byte leaked into the record and left literal_eval parsing '1\xc3\xa92' after: 3it in counting mode, 6it with --update, at every buffer size The stdout payload is byte-for-byte unchanged, and single-byte delimiters are unaffected since they cannot straddle a boundary. New test tests_main_delim_multibyte_split covers the counting and --update paths at three buffer sizes and asserts the payload is preserved. Reverting cli.py makes it fail with 2it. The full suite passes: 174 passed, 5 skipped (numpy, dask and keras are not installed here).
|
I ran into the same undercount while reading One thing worth flagging for whoever reviews this: #1848 fixes the same issue (and the |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix multi-byte delimiters split across read buffers
Fixes #1847.
posix_pipesearched only the fresh chunk from each read:When the delimiter is multi-byte and
--buf_sizeis smaller than it, the delimiter's bytes land in two consecutive reads, so neither chunk contains it. The saved prefix is appended without ever being searched, and the delimiter is missed.Searching
buf + tmpfinds it. Becausebufis non-empty in exactly the case that matters, the indices have to refer to the concatenation:The record passed to the callback must also come from
combined, and the leftover chunk iscombined[i + len_delim:]. Usingtmp[i + len_delim:]would silently drop the bytes still sitting inbuf— which is what a partial fix that only changes thefind()does. That version fixed the count but still mis-sliced the record, and it regressed one buffer size; the full form below is correct at every size.Reproduced
With a 2-byte delimiter and
--buf_size 2:The crash is the same root cause: the first delimiter byte leaked into the record, so
literal_evalwas handedb'1\xc3\xa92'.After
--buf_size--update3it6itThe stdout payload is byte-for-byte unchanged, and single-byte delimiters are unaffected since they cannot straddle a boundary.
Tests
tests_main.py::test_main_delim_multibyte_splitcovers the counting and--updatepaths at three buffer sizes and asserts the payload is preserved. Revertingcli.pymakes it fail with2it.Full suite: 173 passed, 7 skipped (the skips are
numpy,rich,daskandkeras, none of which are installed in this environment).