Remove Guarantee to Deregister on Drop

[cramertj]

In the documentation for Poll there's a note that "Evented handles are automatically deregistered when they are dropped. It is common to never need to explicitly call deregister." If deregistration requires access to the selector, implementing this guarantee requires that Evented implementations keep a Weak<Selector> or similar. Especially in the case of event loops such as tokio-core, it doesn't seem too burdensome to require that they manually deregister Eventeds prior to dropping them. Changing this guarantee would allow for more efficient Evented implementation.

[carllerche]

Given that epoll itself does not make this guarantee (if the FD is duped, epoll only deregisters once all the dups have been dropped), I think that it would be reasonable to require manual calls to deregister in order to guarantee not getting further events for that FD.

[alexcrichton]

I don't think this can be done backwards compatibly, as tokio-rs/tokio-core#275 shows external code in the wild at least is relying on on the behavior that drop is equivalent to deregistration.

Put another way, Tokio shouldn't have to call some APIs, for example, to avoid memory leaks. Any drop of an I/O object (abnormal via panics or normal via returning in a function) should clean up all resources, including those transitively associated with an instance of Poll.

[cramertj]

@alexcrichton it's definitely a breaking change to mio, but IMO it's worth it since it prevents extra signals from epoll, and prevents the Fuchsia and Windows Eventeds from having to store a Weak ptr to the selector in order to deregister and prevent memory leaks.

[twmb]

w.r.t. epoll, the first drop actually prevents the fd from every truly being deregistered. Drop closes the underlying fd, whereas deregister explicitly calls epoll_ctl(fd, del...) (see https://github.com/carllerche/mio/blob/e73a6489ddddfa618388f5ccd86d1fa346600b7b/src/sys/unix/epoll.rs#L137)

For some demonstrating code, see:

extern crate mio;
extern crate libc;

use std::io::{self, Write};
use std::os::unix::io::{RawFd, FromRawFd};
use std::fs::File;

use mio::*;
use mio::unix::EventedFd;

fn pipe() -> io::Result<(RawFd, RawFd)>{
    let flags = libc::O_NONBLOCK | libc::O_CLOEXEC;
    let mut pipes = [0; 2];
    unsafe {
        if -1 == libc::pipe2(pipes.as_mut_ptr(), flags) {
            return Err(io::Error::last_os_error());
        }
    }
    Ok((pipes[0], pipes[1]))
}

fn main() {
    let (rfd, wfd) = pipe().unwrap();
    let mut wf = unsafe { File::from_raw_fd(wfd) };
    wf.write(b"a").unwrap();

    let r_tok = 0;
    let poll = Poll::new().unwrap();
    poll.register(&EventedFd(&rfd), Token(r_tok), Ready::readable(), PollOpt::level()).unwrap();

    let rfd2 = unsafe { libc::dup(rfd) };
    unsafe { libc::close(rfd) };


    let mut events = Events::with_capacity(1024);

    poll.poll(&mut events, None).unwrap();
    for event in events.iter() {
        println!("{:?}", event.token());
    }
}

So, to be truly correct and cover dup calls, deregister must be explicitly called.

[alexcrichton]

@cramertj all this is doing though is moving the Weak, it's either stored in mio's objects or it's going to be stored in Tokio's objects. If it's not stored in mio's objects the "naive" usage of the API can likely very easily run into memory leaks (!)

[twmb]

Sorry, to clarify: the first drop only prevents deregistration if the fd has already been duplicated. If there is no duplicate, the underlying fd close does deregister from epoll automatically.

[cramertj]

@alexcrichton Oh, because IoToken only has access to a Handle when it is created, not when it is dropped and when drop_source is called. I thought we could just add an &Handle argument to drop_source, but I see now that PollEvented only has a Remote, not a Handle.

[alexcrichton]

@cramertj ah yeah today it would be very difficult to do but with tokio-rs/tokio-rfcs#3 you only need a Remote to deregister objects and PollEvented will have access to that

[cramertj]

@alexcrichton Okay! I think it seems reasonable to just wait for the new version of tokio then, depending on how far away you think that is (I'm imagining <6 months or so? Is that too ambitious? Probably).

[carllerche]

@cramertj given that this would require a v0.7 bump, it will be a bit tricky.

Tokio exposes Evented as part of its public API, so the question is going to be, can Mio make this change in 0.7 while letting Mio 0.6 re-export Mio 0.7's Evented trait...

Given that Evented also depends on Poll, I'm going to say that I believe the answer is no. That means that we will have to coordinate tokio-rs/tokio-rfcs#3 w/ Mio 0.7.

[cramertj]

@carllerche Yeah, it seems like we're building up a cascade of breaking changes across mio/futures/tokio-XX that will all have to happen at the same time. My hope was that we could change tokio-core now to expect this behavior (even before a breaking change in mio) but it seems like that won't work out because of the differences between Handle and Remote.

[carllerche]

That said, I think we would have to say something like dropping a handle (fd backed value) without calling deregister is undefined.

Specifically, Poll may or may not continue yielding events for that FD.

This is basically epoll's behavior out of the box, so it seems fine to define Mio the same.

[carllerche]

Making this change would "solve" #361.

[Thomasdezeeuw]

On master event::Source (previously Evented) mentions that the source need to be de-registered before dropping. Based on that I'm closing this, if anyone thinks anything else needs to be done please reopen.