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

Consider removing Core::id. #39

Closed
carllerche opened this issue Nov 19, 2017 · 5 comments
Closed

Consider removing Core::id. #39

carllerche opened this issue Nov 19, 2017 · 5 comments

Comments

@carllerche
Copy link
Member

I'm not sure exactly why it was added in the first place. Specifically, I know it was added to provide some token to use as a map key, but I don't recall the exact scenario that required this to be in the library.

The implementation also doesn't handle usize wrapping, which is a very edge case, but it seems like it should be handled.

Basically, enough has changed w/ the usage patterns of how the reactor should be used that I would like to remove Core::id from the initial release and re-evaluate if it is needed.

@carllerche carllerche added this to the v0.1 - Initial release milestone Nov 19, 2017
@aturon
Copy link
Contributor

aturon commented Nov 20, 2017

Fine by me.

@alexcrichton
Copy link
Contributor

This was originally added in tokio-rs/tokio-core#149 to solve tokio-rs/tokio-core#147 which was in turn targeted at alexcrichton/tokio-signal#6 which removed the need for keeping the original event loop alive once you're creating signal handlers.

With no ability to spawn this doesn't really make any more sense as the whole point was to "spawn a thing per event loop" or "spawn one thing globally and only one". That being said I have no idea how this will actually get implemented for signal handling...

@carllerche
Copy link
Member Author

I'm not sure what exactly requires one task per executor in order to handle signals (which is what it seems like it is trying to do).

My initial thought would be that logic that pushes signals to the associated streams would be handled the same way as timers.

@fa7ca7 fa7ca7 mentioned this issue Nov 20, 2017
@aturon
Copy link
Contributor

aturon commented Nov 21, 2017

In any case it seems prudent to start without it at first, and then revisit tokio-signal; it's easy to add back if needed.

@carllerche
Copy link
Member Author

Fixed by #41

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

No branches or pull requests

3 participants