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()’s return on timeout not documented #1600

Closed
danielparks opened this issue Jul 31, 2022 · 10 comments · Fixed by #1603
Closed

poll()’s return on timeout not documented #1600

danielparks opened this issue Jul 31, 2022 · 10 comments · Fixed by #1603

Comments

@danielparks
Copy link

The documentation for the poll() method doesn’t say what it returns when the timeout elapses. (It returns Ok(()).)

I naively expected it to return something like Err(std::io::Error::from(std::io::ErrorKind::TimedOut)). I see that it behaves like the select() and poll() syscalls though, so maybe I shouldn’t have been surprised.

It would be good to document this, including whether or not you can rely on the fact of the events collection being empty to indicate that it timed out.

@Thomasdezeeuw
Copy link
Collaborator

The documentation for the poll() method doesn’t say what it returns when the timeout elapses. (It returns Ok(()).)

I naively expected it to return something like Err(std::io::Error::from(std::io::ErrorKind::TimedOut)). I see that it behaves like the select() and poll() syscalls though, so maybe I shouldn’t have been surprised.

Yep, we try to provide an API similar to what to OS provides to reduce surprises (ironically introducing a surprise here).

It would be good to document this, including whether or not you can rely on the fact of the events collection being empty to indicate that it timed out.

I'm not sure about this. I don't know if any, let alone all, OSs require their poll calls to only return zero events in case of a timeout.

What is your use case that you need to know whether or not the timeout is hit?

@danielparks
Copy link
Author

I’m using this to implement an option to kill a process when it doesn’t provide output for a given amount of time.

These are the two questions that I can’t answer based on the documentation (not asking you to answer them; just explaining my thinking):

  1. How should I treat poll() returning without any events? Right now this situation isn’t strictly defined.
  2. Is it worth tracking the elapsed time for the poll() call to determine if the timeout has expired, or shall I just assume that no events means that it ran into the timeout?

So, the real issue is that the behavior is undefined. Of course, that’s somewhat hard to avoid when you’re abstracting a bunch of different implementations.

I can do some research into how the underlying implementations handle timeouts and returns. I should be able to get to it by next Monday, August 8.

@Thomasdezeeuw
Copy link
Collaborator

I’m using this to implement an option to kill a process when it doesn’t provide output for a given amount of time.

These are the two questions that I can’t answer based on the documentation (not asking you to answer them; just explaining my thinking):

1. How should I treat `poll()` returning without any events? Right now this situation isn’t strictly defined.

Currently Mio doesn't do anything special here, so whatever the OS returns so do we. This currently means that no events means the timeout is hit.

2. Is it worth tracking the elapsed time for the `poll()` call to determine if the timeout has expired, or shall I just assume that no events means that it ran into the timeout?

I don't thing tracking the time is worth is for Mio in general. I think you're better of with assuming that no events means the timeout is hit.

So, the real issue is that the behavior is undefined. Of course, that’s somewhat hard to avoid when you’re abstracting a bunch of different implementations.

It's unspecified, but yes. I think we can safely document that no events means the timeout is hit, but I don't know for sure.

I can do some research into how the underlying implementations handle timeouts and returns. I should be able to get to it by next Monday, August 8.

👍

@danielparks
Copy link
Author

So, a few clarifications to start. I think there are ideally two things that the mio docs should answer:

  1. What does poll() return when it hits a timeout?
  2. Is poll() guaranteed to return Ok(()) and 0 events only when it hits a timeout?

TL;DR: 1. Ok(()) and no events, 2. no. See the bottom of the comment for my proposal.

epoll_wait(2) Return Values

On success, epoll_wait() returns the number of file descriptors
ready for the requested I/O, or zero if no file descriptor became
ready during the requested timeout milliseconds. On failure,
epoll_wait() returns -1 and errno is set to indicate the error.

  1. It returns no events on timeout. You can see in the mio epoll code that it will return OK(()) and clear the events vec.
  2. There is no guarantee that the only time it returns 0 events is on timeout, though I suspect that is true.

kqueue(2) Return Values

The kevent() system call returns the number of events placed in the
eventlist, up to the value given by nevents. If an error occurs while
processing an element of the changelist and there is enough room in the
eventlist, then the event will be placed in the eventlist with EV_ERROR
set in flags and the system error in data. Otherwise, -1 will be re-
turned, and errno will be set to indicate the error condition. If the
time limit expires, then kevent() returns 0.

  1. It returns no events on timeout. You can see in the mio kqueue code that it will return OK(()) and clear the events vec.
  2. There is no guarantee that the only time it returns 0 events is on timeout, though I suspect that is true.

IOCP

…I looked at the docs briefly and decided it was going to be way too much work to understand IOCP.

However, I took a look at the mio code for IOCP and in the event of a timeout, it never adds any events to the events vec (after clearing it earlier).

This code suggests that it might be possible to get no events if an timeout is specified, but doesn’t elapse.

  1. Ok(()) and 0 events on timeout.
  2. The code suggests that there may be other situations where it returns Ok(()) and 0 events.

WASI

I can’t find a decent spec for this, but looking at the mio wasi code I suspect that a timeout will result in 0 events. Presumably if the timeout hits and an event hits at the same time, then the timeout is irrelevant.

  1. Probably Ok(()) and 0 events on timeout.
  2. No idea if other situations might return the same thing.

Proposal

I would follow the lead of epoll(2) and kqueue(2). I would add a single sentence paragraph to the poll() docs just before the paragraph that starts “Note that the timeout…”:

If timeout elapses, poll returns Ok(()) and events will be empty.

If that works for you I can prepare a PR.

@Thomasdezeeuw
Copy link
Collaborator

Proposal

I would follow the lead of epoll(2) and kqueue(2). I would add a single sentence paragraph to the poll() docs just before the paragraph that starts “Note that the timeout…”:

If timeout elapses, poll returns Ok(()) and events will be empty.

I'm ok with documenting the current situation, but I don't think we can be this decisive. We're simply following what the OS does, so we can document that, but I don't think we can guarantee (or state in the docs) that events will always be empty if we hit a timeout.

It's think it would be better describe that currently all supported OSs return Ok(()) and no events, but we're not limiting ourselves to that.

@danielparks
Copy link
Author

I'm ok with documenting the current situation, but I don't think we can be this decisive. We're simply following what the OS does, so we can document that, but I don't think we can guarantee (or state in the docs) that events will always be empty if we hit a timeout.

That’s not true, though. IOCP returns an error on timeout, and mio swallows that, returns OK(()), and clears the events vec.

It seems like a very bad idea to make code the uses mio handle something as basic as timeouts in different ways depending on the underlying implementation.

@Thomasdezeeuw
Copy link
Collaborator

I'm ok with documenting the current situation, but I don't think we can be this decisive. We're simply following what the OS does, so we can document that, but I don't think we can guarantee (or state in the docs) that events will always be empty if we hit a timeout.

That’s not true, though. IOCP returns an error on timeout, and mio swallows that, returns OK(()), and clears the events vec.

Right, but that's to match the behaviour of Unix (kqueue and epoll). But neither kqueue or epoll guarantee that it can't timeout and return an events, even though they usually always return when an event is found. Note the keyword gurantee vs. 90% of the time here, like I said before I'm ok with documenting it in general.

It seems like a very bad idea to make code the uses mio handle something as basic as timeouts in different ways depending on the underlying implementation.

Mio has many subtle difference we can't always smooth over, it's simply the nature of a low-level crate. We always do our best to document them though.

@danielparks
Copy link
Author

What I’m proposing is documenting the current situation. What about just saying:

If timeout elapses, poll returns Ok(()).

At core, that resolves the issue I have with this. As a consumer of the library, I need to know what mio does when it times out.

@Thomasdezeeuw
Copy link
Collaborator

What I’m proposing is documenting the current situation. What about just saying:

If timeout elapses, poll returns Ok(()).

At core, that resolves the issue I have with this. As a consumer of the library, I need to know what mio does when it times out.

And I agree with documenting the current situation, just not with guaranteeing it. That's the difference we're talking about.

@danielparks
Copy link
Author

Ok, so what do you agree is the current situation? I just want to know so I (or somebody else, whatever) can submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants