PollEvented needs robust fd {de,re}registration

[ipetkov]

PollEvented does not explicitly deregister its io source on drop, which can lead to problems with registering reused file descriptors.

I found a situation in my project where a descriptor was duplicated, registered with the event loop via PollEvented, and then dropped after some work was done. A little bit later, a file descriptor was opened and happened to coincide with the same fd that was registered before. Upon registration of the "new" fd via PollEvented, EEXISTS was returned by epoll_ctl(EPOLL_CTL_ADD) (whose propagation effectively cancelled my future). Haven't noticed this happening on macOS which relies on kqueue (I believe its API is more forgiving of reregistration of existing fds).

Seems like if PollEvented or the Core loop could ensure sources are deregistered on drop it would solve the most amount of problems (though the Remote to Handle coersion could cause issues). Alternatively EEXISTS could be caught and retried via reregister but I'm not sure if that would end up masking other significant problems.

[dwrensha]

Related to tokio-rs/mio#361. Unfortunately, epoll has weird semantics for dup()ed file descriptors.

[alexcrichton]

Thanks for the report! Currently it's an intentional design decision that PollEvented doesn't deregister on drop because this allows PollEvented<E> to not require E to be Send (to send the I/O object back to the reactor for deregistration). The thinking is that typically PollEvented owns the I/O object in question so when PollEvented is dropped the I/O is dropped, automatically deregistering anyway.

If you're working with custom file descriptors though maybe deregister could be called manually? (with proof you're on the same thread).

[ipetkov]

Thanks for the info @alexcrichton! I did consider explicitly calling deregister but had some concerns if the future isn't on the same thread. Thinking about it a bit more it should fit my use case, though I have one question: are tasks created via Remote::spawn guaranteed to always be spawned/initial-polled in order of creation? If so, that should be all I need to get unblocked! (though it would be unfortunate for all consumers of PollEvented to have to write their own bridge to deregister...)

The thinking is that typically PollEvented owns the I/O object in question so when PollEvented is dropped the I/O is dropped, automatically deregistering anyway.

This is exactly how I'm using PollEvented (by passing in an owned wrapper around a file descriptor). It appears that on drop only the PollEvented's token gets dropped from the event loop (so it won't react to it any more), however, the file descriptor is not physically removed from epoll which misled me for a while.

[alexcrichton]

I think that spawn guarantees in-order receiving of messages, and that seems like a reasonable guarantee to provide to me!

Also yeah if you're working with dup'd file descriptors I think that file descriptions are associated with epoll (not descriptors), so closing a dup'd handle wouldn't auto-unregister.

[ipetkov]

Ah the classic dup pitfall... The epoll man-page confirms that once all duplicates of a file descriptor are closed it is automatically deregistered (apologize for not having done more research on my own earlier), and that it uses the combination of the file descriptor and description to distinguish entries, which is the exact condition I was hitting.

I'll take out the superfluous dup-ing from my code, but do you think it is still worth having PollEvented::new catch EEXIST errors and succeed on them anyway? All in all, EEXIST is pretty much a sign that the registration is already there and we shouldn't have to propagate the error. Otherwise it seems impossible to create a new PollEvented wrapper around that particular file descriptor without ensuring the previous instance was explicitly deregistered (which is cumbersome on its own).

[ipetkov]

Took a closer look at the internals of event registration in mio. I was hoping that using Evented::reregister would get around the EEXIST issue, but since mio only supports associating a single token with a single file descriptor, using reregister will overwrite any previous token. This would cause actual problems if a second PollEvented wrapper is created with the same inner value (probably starving the first instance causing debugging nightmares).

I'm good with ensuring a call to deregister on my end since that's the best thing worth doing today. Thanks again for the clarifications, @alexcrichton, feel free to close this unless there are any design considerations that can be gleaned here!

[alexcrichton]

@ipetkov you can see how tokio-curl works with this error here

I'd be wary of ignoring errors (even these), but would that strategy work for you?

[ipetkov]

@alexcrichton yes, I completely forgot about the Evented bridge, definitely seems like the right place to do this sort of thing (if needed and warnings considered). Thanks!