Fix decoding error (issue 168) - #169
Conversation
|
@christian-intra2net Thanks for contributing! Before any review though - have you run the unit tests? I believe they also need adaptation. |
|
Oops, sorry, I was not aware of unit tests. I think I found the problem, fixing it now... |
4821842 to
2f9de5d
Compare
|
Great to see the CI passing, to clarify for everyone else - this PR fixes issue #168. |
pevogam
left a comment
pevogam
left a comment
There was a problem hiding this comment.
This is an initial review from me, overall this pull request fixes a really non-trivial issue!
| raw_data = os.read(expect_pipe, 1024) | ||
| if not raw_data: | ||
| return read, data | ||
| return read, data.decode(self.encoding, "ignore") |
There was a problem hiding this comment.
I assume from your comment in #168 (comment) this could be instead be turned into replace and thus provide the clarity you mentioned there. So let's see if we collect some feedback there on the original choices first and until then a "replace" setting here would rather be a requested change for additional improvement there here.
There was a problem hiding this comment.
I'd suggest another commit to do mass ignore->replace which could be reverted in case someone depended on the ignore.
| return read, data.decode(self.encoding, "ignore") | ||
| read += len(raw_data) | ||
| data += raw_data.decode(self.encoding, "ignore") | ||
| data += raw_data |
There was a problem hiding this comment.
Definitely better not to decode raw data until the very end, I think this change improves the clarity and related better to the choice of naming.
| thread.join() | ||
|
|
||
|
|
||
| def partial_decode(input_bytes, encoding): |
There was a problem hiding this comment.
Maybe we could add a few unit tests for what behavior should be contracted with this function?
There was a problem hiding this comment.
Maybe there is also a better location for it e.g. in utils folder or something like that since the current module is purely structuring the classes in order of composition.
There was a problem hiding this comment.
yep, utils.astring would be IMO the best location
| return text, b"" | ||
|
|
||
| # otherwise, we return the bytes after the last good char | ||
| return text, input_bytes[index + len(last_understood) :] |
There was a problem hiding this comment.
This seems quite complex, how about:
def partial_decode(input_bytes, encoding):
"""
Helper for decoding as many bytes as possible, returning last "broken"
bytes.
:param input_bytes: Encoded input text
:param encoding: Target encoding
:returns: tuple(encoded text, left-over bytes)
"""
decoder = codecs.getincrementaldecoder(encoding)(errors="ignore")
text = decoder.decode(input_bytes, final=False)
leftover, _ = decoder.getstate()
return text, leftoverThere was a problem hiding this comment.
(and ideally in utils.astring as it's string manipulation)
ldoktor
left a comment
ldoktor
left a comment
There was a problem hiding this comment.
Thanks and kudos for the analysis. I'd suggest using the incremental decoder instead but apart from that it's really welcome bugfix. Please include a test for various cases.
|
You are completely right, I did not know about the IncrementalDecoder, so I basically re-implemented its functionality. Using such an object, we can get rid of much more of my code, I will make the modifications early next week. |
By decoding incomplete byte-input, we risk losing multi-byte characters if their byte-representation is not aligned with the end of our buffer. Fix this by concatenating bytes first, and only decode when we actually have to. In case of `Tail`` that is a little complicated, because we need to return text in-between `read()` calls. Outsource to a helper function with lots of documentation to clarify and reduce complexity. A reviewer noticed that the functionality included in partial_decode has a big overlap with that of python's own IncrementalDecoder. In fact, I basically partially re-implemented it. Simplify the PR a lot by using codecs.getincrementaldecoder. Since this is a bit shaky in python 3.13 we call the decode() function with keyword argument. Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
2f9de5d to
a780037
Compare
I did that, reduces the code changes a lot
I created a stand-alone test-script for development and testing but I guess you'd prefer some kind of unittest. It would have to spawn some process that produces byte-output, at least for linux I could piece that together. |
b78a850 to
ba050a8
Compare
Check fix we introduced with last 2 commits by producing long multibyte text with various offsets, so some os.read() of it will encounter incomplete characters. Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
ba050a8 to
c985062
Compare
|
I have converted my stand-alone test script to a unittest. It tests both code locations that were changed, filling the buffers of Tail and ShellSession with multibyte character strings of different alignments to force the issue of incomplete reads. In my tests, these tests succeed on the current branch but fail on main (after replacing the new buffer size const with a literal 1024). Thanks @pevogam for pushing this PR |
3 participants
By decoding incomplete byte-input, we risk losing multi-byte characters, if their byte-representation is not aligned with the end of our buffer.
Fix this by concatenating bytes first, and only decode when we have to.
In case of Tail that is a little complicated, because we need to return text in-between read() calls. Outsource to a helper function with lots of documentation to clarify and reduce complexity.
Clarify error policy for decode: why not switch from "ignore" to "replace" to notify callers of decoding problems instead of hiding them?
Original author: Christian Herdtweck christian.herdtweck@intra2net.com