Interval is Infallible

[cramertj]

Interval::poll never returns an error, but its associated error type is io::Error. The error type should be changed to enum Never {} or similar (eventually ! when it lands).

[alexcrichton]

Thanks for the report! I think we'll need an error here in the long run, however. It's possible for the core/reactor to disappear while the Interval itself is active, and I think we'd want that to resolve to an error?

[cramertj]

Couldn't the same be said of any tokio-core-based Future? IIRC they will all fail to resolve if the reactor disappears. I don't see how you could ever prevent this, as the reactor could be dropped after a timeout has been scheduled, but before it has triggered.

[alexcrichton]

You're right yeah, but futures with embedded handles are slightly different. If you spawn a future you're not holding on to it at all, but if you create an interval that's a handle to the reactor. In that sense we don't have a way to tell you the reactor is dropped when you spawn a future, but we do have a method of telling you once you create a handle to the reactor.

[cramertj]

I guess I'm not convinced that it's worth it to force all users to accept these unhandle-able io::Errors in order to signal that the reactor has been dropped. Most if not all async applications using tokio will keep a single reactor core alive for the duration of the program (especially after the new tokio RFC), so it seems odd to force every future to handle this error case.

If we decide that it is worth it, I think it'd be better to provide an error type that is more clear about what's happening, something like struct ReactorClosed;.

[alexcrichton]

Is this actually causing problems? In all the usage I've personally done of these types the error fits quite nicely into the surrounding future and rarely requires much fuss to get it to work. I agree that the reactor being dropped is quite rare, but I didn't think this was a problem to begin with :(

[cramertj]

IMO, forcing users to map_err in handle.spawn(interval.for_each(|()| ...).map_err(|_| ())) is unfortunate. It suggests that users should be properly handling or logging an error, which is misleading since an error can never actually occur. In a world where spawn took a Future<Item = (), Error = Never> (or a spawn_infallible was added), the map_err wouldn't even be necessary.

I'm fine with closing this as "won't fix" or similar, but I'd like to at least add some documentation explaining that the error case here doesn't represent an actual IO error.

For reference, this came up because I was asking a coworker to log errors instead of just mapping them away to (), and we were both wondering what sorts of io::Error could actually be generated by an Interval.

[carllerche]

I imagine that this discussion would be more relevant to the ongoing Tokio revamp RFC as it impacts timers.

[alexcrichton]

@cramertj hm I'm not sure I quite understand, if you do interval.for_each(..) doesn't it not matter what error type the interval has, if any? The for_each closure will add an error type regardless?

I agree though that this should certainly be documented what errors can come up!

[cramertj]

@alexcrichton for_each's error type is S::Error, where S is the original input stream.

[alexcrichton]

Hm I think I may still be missing something? I don't think we can get away with a "void error type" here because the error can legitimately happen, and in that case aren't you required to deal with some error? I'd imagine that "some error" vs "no error" is a pretty big distinction to make, but given that I think we should buy into "some error" (due to the reactor/timer going away) it'd necessitate some level of error handling, and then following that further I think io::Error is probably the most ergonomic as it tends to be mixed up in I/O anyway.

[cramertj]

I don't think we can get away with a "void error type" here because the error can legitimately happen

I guess that's the real answer to my question here-- I wasn't sure this was true. The only reasonable way that Interval::poll could return an error was if one of these handle.send calls failed here and that error was bubbled back up. According to the UnboundedSender docs, this error will occur when the receiving end of the channel (the reactor core) is dropped. I don't think that's a case we can or should handle gracefully-- delivery of error messages in this case is race-y at best, since the reactor could be dropped immediately after a send call without the future's task ever being notified again.

[cramertj]

since the reactor could be dropped immediately after a send call without the future's task ever being notified again.

I suppose you could always notify and all futures again when the reactor is dropped, but after you've closed the UnboundedReceiver. This would allow futures to gracefully respond to the reactor closing. If this is the intention, I'm totally fine with closing this and just adding some documentation explaining that Interval will only error if the reactor core has been dropped.

[alexcrichton]

@cramertj

I don't think that's a case we can or should handle gracefully-- delivery of error messages in this case is race-y at best

You are indeed right! I suppose I'm trying to think of this wrt the futures as well, where for example I believe the implementation at https://github.com/alexcrichton/futures-timer correctly handles this. (that's my current plan for reimplementing this repo's interval in terms of).