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

properly handle read timeouts with use_poll=True #492

Merged
merged 1 commit into from May 24, 2018

Conversation

Projects
None yet
3 participants
@ryanpetrello
Contributor

ryanpetrello commented May 21, 2018

see: #491

@ryanpetrello

This comment has been minimized.

Contributor

ryanpetrello commented May 21, 2018

@ryanpetrello ryanpetrello force-pushed the ryanpetrello:fix-poll-timeouts branch from 290b65e to 7cd4101 May 21, 2018

@@ -165,7 +165,7 @@ def poll_ignore_interrupts(fds, timeout=None):
poller = select.poll()
for fd in fds:
poller.register(fd)
poller.register(fd, select.POLLIN | select.POLLPRI | select.POLLHUP | select.POLLERR)

This comment has been minimized.

@ryanpetrello

ryanpetrello May 21, 2018

Contributor

The prior version of this was returning immediately on select.POLLOUT events (Ready for output: writing will not block). I think we only care about read events here, given that the goal is to be notified when new data is ready for the read_nonblocking call at:

return super(spawn, self).read_nonblocking(size)

This comment has been minimized.

@cooperlees

cooperlees May 21, 2018

Contributor

Great catch. Makes sense to me.

@cooperlees

cooperlees approved these changes May 21, 2018 edited

Many thanks for this! I think we should be more explicit with your test name and we're good.

  • Lets give a little bit of time for others to review too tho.
@@ -87,6 +87,12 @@ def test_read_poll(self):
remaining = child.read().replace(_CAT_EOF, b'')
self.assertEqual(remaining, b'abc\r\n')
def test_read_timeout(self):

This comment has been minimized.

@cooperlees

cooperlees May 21, 2018

Contributor

NIT: test_read_poll_timeout

@@ -165,7 +165,7 @@ def poll_ignore_interrupts(fds, timeout=None):
poller = select.poll()
for fd in fds:
poller.register(fd)
poller.register(fd, select.POLLIN | select.POLLPRI | select.POLLHUP | select.POLLERR)

This comment has been minimized.

@cooperlees

cooperlees May 21, 2018

Contributor

Great catch. Makes sense to me.

@ryanpetrello ryanpetrello force-pushed the ryanpetrello:fix-poll-timeouts branch from 7cd4101 to d57a6d9 May 21, 2018

@takluyver

This comment has been minimized.

Member

takluyver commented May 21, 2018

Yup, makes sense to me. You're right that POLLOUT events shouldn't be important to us.

@ryanpetrello

This comment has been minimized.

Contributor

ryanpetrello commented May 22, 2018

@takluyver whenever this merges, would you mind cutting a new release on PyPI 😄?

@takluyver

This comment has been minimized.

Member

takluyver commented May 23, 2018

Yup, give or take merging a couple of other PRs that are going on at the moment.

@cooperlees feel free to merge this one whenever you think it's had long enough for people to review it. :-)

@cooperlees

This comment has been minimized.

Contributor

cooperlees commented May 23, 2018

I would, but I do not have commit/merge access!

@takluyver

This comment has been minimized.

Member

takluyver commented May 24, 2018

Oh, right, sorry, I forget who's a maintainer...

@takluyver takluyver merged commit 50205e3 into pexpect:master May 24, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 88.614%
Details
@takluyver

This comment has been minimized.

Member

takluyver commented May 24, 2018

#490 is the other one that I'll try to merge before doing a release.

@takluyver

This comment has been minimized.

Member

takluyver commented May 24, 2018

If anyone wants to help towards the release, a PR to start updating the release notes in the docs would be welcome. :-)

@ryanpetrello

This comment has been minimized.

Contributor

ryanpetrello commented May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment