bpo-34215: Update streams.py:readuntil IncompleteReadError issue by mrbell321 · Pull Request #4354 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits3 Checks0 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
When IncompleteReadError is raised the message is IncompleteReadError: 1 bytes read on a total of None expected bytes
But None is incorrect. The correct expected length is seplen
https://bugs.python.org/issue34215
Hello, and thanks for your contribution!
I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).
Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.
Thanks again to your contribution and we look forward to looking at it!
You should first create an issue on bugs.python.org
Fixing test_readuntil_eof to comply with separator expected length
Well... 9 months later, I created an account, signed the CLA and created an issue(#34215) in bugs.python.org.
Sorry about the delay...
Update streams.py:readuntil IncompleteReadError Update streams.py:readuntil IncompleteReadError issue number 34215
Update streams.py:readuntil IncompleteReadError issue number 34215 Update streams.py:readuntil IncompleteReadError issue bpo-34215
Update streams.py:readuntil IncompleteReadError issue bpo-34215 bpo-34215: Update streams.py:readuntil IncompleteReadError issue
@mrbell321, in order to move forward with this pull request, please rebase to resolve the conflicts. Also, it looks like the CI's all failed initially, so the rebase should retrigger the tests. If they fail again, please correct the errors. Thanks!
Let's imagine the case where seaprator='\n', seplen=1.
We read a chunk if 300 bytes.
The chunk doesn't contain a separator.
The stream is closing now by peer.
Currently, the message is IncompleteReadError: 300 bytes read on a total of None expected bytes which is correct.
Proposed IncompleteReadError: 300 bytes read on a total of 1 expected bytes is wrong, isn't it?
Well I'm not sure the general text of IncompleteReadError matches this use case very well.
I think of it this way: if the separator is \n and I'm reading until that separator, I expect to match one byte so 1 makes more sense than None to me because I expected at least 1 byte to match, but it didn't. As it originally was, the message seems to indicate that we didn't expect to see anything at all which is not true.
Perhaps this bug should be resolved in different wording of the IncompleteReadError text instead? Something along the lines of IncompleteReadError: {len(chunk)} bytes read, {None} matched on a total of {seplen} expected bytes ?
Perhaps this bug should be resolved in different wording of the IncompleteReadError text instead? Something along the lines of
IncompleteReadError: {len(chunk)} bytes read, {None} matched on a total of {seplen} expected bytes?
The text is weird for reader.readexactly(100) call if the stream is closed by the peer after reading 50 bytes only.
class IncompleteReadError(EOFError):
"""
Incomplete read error. Attributes:
- partial: read bytes string before the end of stream was reached
- expected: total number of expected bytes (or None if unknown)
"""
def __init__(self, partial, expected):
super().__init__("%d bytes read on a total of %r expected bytes"
% (len(partial), expected))
self.partial = partial
self.expected = expected
Ok, so the docstring indicates that None means that we don't know the total number of bytes, so I guess IncompleteReadError: 300 bytes read on a total of None expected is correct, even if it's not obvious).
I may change this PR to address the text of the exception: ("%d bytes read on a total of %r expected bytes" % (len(partial), "unknown" if expected is None else expected)) or maybe a separate exception with more appropriate text for readuntil?
I'll have to track down the code that I discovered this through(I don't even remember at this point) and put a try/catch/raise there too because I think there's an issue there w/ hiding the call to readuntil, but exposing the exception.
I thought about proposing "undefined" just before reading your comment :)
Sorry about the noise. I'm having trouble running the tests locally.
Tyler Bell and others added 2 commits
@mrbell321, are you still interested in pursuing the last comments you made about possible changes to this PR? Thanks!
Stale PR or inactive for long period of time.
label
I'm going to close this as inactive. It can be reopened if the original author is interested in working on it or a new PR can be opened to replace it. If the original author's work is used in a new PR, please credit them as a co-author.