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

Poll::poll(): Retry select() on EINTR #742

Merged
merged 1 commit into from Nov 7, 2017

Conversation

Projects
None yet
3 participants
@asayers
Copy link
Contributor

asayers commented Oct 5, 2017

Usually the user should retry poll() on EINTR; however, this is an
easily-overlooked case. Let's improve the reliability of mio-based
programs by handing EINTR by default.

Closes #494.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Oct 5, 2017

Thanks. I believe that the timeout would need to be updated before poll is attempted again?

@asayers

This comment has been minimized.

Copy link
Contributor Author

asayers commented Oct 11, 2017

Sorry for the slow response. Yes, you're right.

@asayers asayers force-pushed the asayers:eintr_retry branch from eba91ff to b1a9eec Oct 11, 2017

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Oct 11, 2017

A CI fix has been merged to master. Would you mind rebasing when you have a moment? Thanks!

@asayers asayers force-pushed the asayers:eintr_retry branch from b1a9eec to 0b4ca9b Oct 11, 2017

@asayers

This comment has been minimized.

Copy link
Contributor Author

asayers commented Oct 12, 2017

Rebased. The iOS CI run timed out after starting the test program. I can't tell if this failure is genuine - is it possible to rerun the test?

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Oct 16, 2017

It probably is spurious. iOS CI is really janky.

@asayers asayers force-pushed the asayers:eintr_retry branch from 0b4ca9b to 3c1882d Oct 17, 2017

@asayers

This comment has been minimized.

Copy link
Contributor Author

asayers commented Oct 17, 2017

Added a line to the changelog.

src/poll.rs Outdated
if let Some(to) = timeout {
let elapsed = now.elapsed();
timeout = if elapsed >= to {
Some(Duration::from_millis(0))

This comment has been minimized.

@carllerche

carllerche Oct 25, 2017

Owner

At this point, would you not break? While probably an implausible situation, if each call to select returns w/ an interrupt, this would cause an infinite loop?

This comment has been minimized.

@asayers

asayers Oct 30, 2017

Author Contributor

I assumed that calling select with a timeout of 0 is non-blocking and therefore can't return EINTR, but to my surprise a bit of googling revealed that "non-blocking syscalls don't return EINTR" is not actually documented behaviour. I'll change to a break.

EDIT: I found an email from David Miller saying that EINTR is impossible for non-blocking syscalls on Linux and (he thinks) on BSD.

Poll::poll: Retry select() on EINTR
Usually the user should retry poll() on EINTR; however, this is an
easily-overlooked case. Let's improve the reliability of mio-based
programs by handing EINTR by default.

Closes #494.

@asayers asayers force-pushed the asayers:eintr_retry branch from 3c1882d to fe4430d Oct 30, 2017

@asayers

This comment has been minimized.

Copy link
Contributor Author

asayers commented Oct 30, 2017

  • Rebased on master
  • Skip retry when timeout has expired
@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Nov 7, 2017

🎉 CI passes.

Thanks for hanging in there.

@carllerche carllerche merged commit 548ed6a into carllerche:master Nov 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asayers

This comment has been minimized.

Copy link
Contributor Author

asayers commented Nov 8, 2017

Thanks!

@bbshelper

This comment has been minimized.

Copy link

bbshelper commented Jan 11, 2018

My application is required to gracefully shutdown itself upon SIGTERM. It does so by changing a global variable in signal handler, and check the variable after poll returns ErrorKind::Interrupted. With this change, poll won't pass that information, breaking my application. Will it be possible to see the old behavior, maybe with a new API?

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Jan 11, 2018

Yeah, it would be fine to add a poll_interruptible. Sorry about that.

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.