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

Tweak the PollEvented::deregister signature #55

Merged
merged 7 commits into from
Dec 5, 2017

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Dec 5, 2017

This commit changes the PollEvented::deregister signature from

fn deregister(self, handle: &Handle) -> io::Result<()>

to

fn deregister(&self) -> io::Result<()>

Now that the handles are Send and Sync there's no longer any need to pass it
in (it's already stored in the PollEvented itself). Additionally this switches
to &self instead of self to allow reclamation of the internal resources if
necessary.

This PR is based on #54, only the last commit needs to be reviewed.

The `Handle` type is now `Send` and `Sync` so the `Remote` type no longer needs
to exist.
No need for oneshot shenanigans as now we'll always have the `Handle` available
to us regardless of what thread we're on to associate a new socket
This is no longer needed now that the public-facing `CoreId` has been removed
This commit renames the various constructors of networking types to have a
`_std` suffix instead of a smorgasboard of other suffixes, canonicalizing on
`_std` as the suffix for constructors which take the libstd corresponding types.
This commit is targeted at solving tokio-rs/tokio-core#12 and incorporates the
solution from tokio-rs/tokio-core#17. Namely the `need_read` and `need_write`
functions on `PollEvented` now return an error when the connected reactor has
gone away and the task cannot be blocked. This will typically naturally
translate to errors being returned by various connected I/O objects and should
help tear down the world in a clean-ish fashion.
This should allow configuration over what reactor accepted streams go on to by
giving back a libstd-bound object that can then be used later in conjunction
with `TcpStream::from_std`.
This commit changes the `PollEvented::deregister` signature from

    fn deregister(self, handle: &Handle) -> io::Result<()>

to

    fn deregister(&self) -> io::Result<()>

Now that the handles are `Send` and `Sync` there's no longer any need to pass it
in (it's already stored in the `PollEvented` itself). Additionally this switches
to `&self` instead of `self` to allow reclamation of the internal resources if
necessary.
@cramertj
Copy link
Contributor

cramertj commented Dec 5, 2017

Nice! Now that it no longer requires any external parameters, should deregister be removed then in favor of a Drop impl? tokio-rs/mio#753 brought up some issues that could be resolved by doing this.

@carllerche carllerche merged commit 4606279 into tokio-rs:new-crate Dec 5, 2017
@alexcrichton alexcrichton deleted the change-deregister branch December 5, 2017 21:47
@alexcrichton
Copy link
Contributor Author

@cramertj perhaps yeah, but I think we'd still want deregister anyway b/c that way (on Unix at least) you could migrate objects between event loops (in theory at least). I've also personally been under the assumption so far that if you drop the PollEvented it internally drops the E which at the OS level implicitly performs deregistration so an explicit call isn't necessary.

That being said tokio-rs/mio#753 would of course change the calculus here quite a lot!

@cramertj
Copy link
Contributor

cramertj commented Dec 5, 2017

@alexcrichton

that way (on Unix at least) you could migrate objects between event loops

I'd assume in order to do that you'd need a register function of some sort which takes a new Handle. If that's the case, you could call deregister internally prior to registering with the new event loop.

I've also personally been under the assumption so far that if you drop the PollEvented it internally drops the E which at the OS level implicitly performs deregistration so an explicit call isn't necessary.

That makes sense, but I think the conversation in tokio-rs/mio#753 and tokio-rs/mio#361 turned up that implicit deregistration doesn't work right if the underlying file descriptor has been duplicated.

@alexcrichton
Copy link
Contributor Author

@cramertj oh I was also thinking we'd have something like into_inner eventually.

but I think the conversation in tokio-rs/mio#753 and tokio-rs/mio#361 turned up that implicit deregistration doesn't work right if the underlying file descriptor has been duplicated.

True, but we don't expose any apis that duplicate underlying file descriptors (precisely for this reason!) so if you get into that situation you're sort of "on your own" at that point anyway.


FWIW I believe the original motivation for a method like this was to deregister file descriptors in response to external runtimes like libcurl provides. Looking at tokio-curl today it doesn't actually use this though so maybe it migrated away at some point!

Regardless though I think for low-level functionality we'll want to retain functionality like this as it seems inevitable to come up at some point, although I could also imagine tweaking the signature of deregister or having more stern warnings about using the PollEvented after deregister has been called.

@cramertj
Copy link
Contributor

cramertj commented Dec 5, 2017

@alexcrichton Okay! Seeing as tokio-rs/tokio-core#282 gets around this by not using PollEvented at all, I will plan to continue doing my own thing for Fuchsia and trust your judgement concerning other platforms :)

@alexcrichton
Copy link
Contributor Author

Heh, true!

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 this pull request may close these issues.

3 participants