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

Fixed overlapped I/O when more data needs to be read #6272

Merged
merged 3 commits into from Jan 31, 2019

Conversation

Projects
None yet
2 participants
@ethanhs
Copy link
Collaborator

commented Jan 30, 2019

For some reason the tests didn't capture this, but ReadFile, not GetOverlappedResult returns _winapi.ERROR_MORE_DATA, which means that we would not do another read (instead we would fail an assert). This handles the case where there is more data to be read and correctly peeks then reads the remaining data.

I also increased the size of the test message to make sure we don't miss errors like this in the future, and added more information when assertions fail.

ethanhs added some commits Jan 30, 2019

Fix overlapped I/O when more data needs to be read
For some reason the tests didn't capture this, but ReadFile, not
GetOverlappedResult returns _winapi.ERROR_MORE_DATA, which means that we
would not do another read. This handles that and correctly peeks then
reads the remaining data.

@ethanhs ethanhs requested a review from msullivan Jan 30, 2019

@msullivan
Copy link
Collaborator

left a comment

Good find!

self.name = name
self.timeout = timeout

def read(self) -> bytes:
def read(self, size: int = 100000) -> bytes:

This comment has been minimized.

Copy link
@msullivan

msullivan Jan 30, 2019

Collaborator

I'm not sure if this is worth putting in the interface, since it doesn't modify any visible behavior.

This comment has been minimized.

Copy link
@ethanhs

ethanhs Jan 31, 2019

Author Collaborator

I just felt it was more fitting to put it in the function scope since it is only used for reads. To me it makes the code easier to understand (one less instance attribute to think about in the other methods).

This comment has been minimized.

Copy link
@msullivan

msullivan Jan 31, 2019

Collaborator

Alright, sure

@msullivan msullivan merged commit 1457c5d into python:master Jan 31, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.