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

Start adding a global event loop #57

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Contributor

This commit starts to add support for a global event loop by adding a
Handle::default method and implementing it. Currently the support is quite
rudimentary and doesn't support features such as shutdown, overriding the return
value of Handle::default, etc. Those will come as future commits.

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

@carllerche
Copy link
Member

Thanks for this.

I took away from the RFC discussion that it was not clear whether or not implicitly starting the global loop on first use would be better than explicitly starting the global event loop.

Specific questions.

  • Is it clearer to the user that Tokio works "by default" with a runtime thread if the user must start the thread explicitly?
  • How does a user prevent the global runtime thread from starting?
  • How does a user shutdown the runtime thread?
  • How does the user start the runtime thread after it has been explicitly disabled? (from point 2).
  • How does the user restart the global runtime thread after it has been shutdown?

I think this PR should be continued to explore this space so these questions can be better addressed.

I will add some more specific feedback inline.

}

#[derive(Clone)]
enum HandleRepr {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this enum should be avoidable if you can create a HelperThread instance without actually starting the thread. You would get the Handle of the reactor that hasn't actually started running. If creating a reactor fails, you could set inner to Weak::new().

Either way, I think being able to get the handle to the global reactor when the thread isn't running is useful in its own right. This goes back to the questions I posed in the above comment.

@aturon
Copy link
Contributor

aturon commented Dec 6, 2017

@carllerche Thanks for laying out those issues. As I expressed when we chatted about this on IRC, I would strongly prefer that Tokio by default lazily spin up a reactor thread, and provide an explicit opt out if you want to avoid this (which is the design in the RFC). This is both to make the defaults match the most common use case/best practices, but also to make it feasible to wrap Tokio-based libraries with synchronous APIs, while keeping Tokio a purely private dependency (i.e., not requiring the app to know about or initialize a Tokio event loop).

I do believe this is purely a question of defaults, and that we should provide the full range of functionality you spell out. That said, it seems feasible and helpful to review the PR as-is, on the basis of the RFC text, and leave the questions you raise open in the tracker for follow up PRs, so that we don't get too bogged down at this juncture.

@carllerche
Copy link
Member

@aturon Well, the RFC doesn't address the questions, so if we need to land things in the RFC first before PRs, maybe we should take this discussion back to the RFC and get that updated (I'm not entirely sure what you are getting at).

@aturon
Copy link
Contributor

aturon commented Dec 6, 2017

@carllerche

The RFC specifies that an event loop thread will be started up automatically, and allows for using make_default_for to register a custom event loop within an executor (which, if used consistently, will prevent the default one from being spun up). It doesn't address shutdown or restart, but my point was that it seems prudent to review and land the core functionality, then follow up on the additional open questions. I'd rather not block review of this PR on doing more design.

@carllerche
Copy link
Member

I guess I should clarify that (as I tried to say on IRC), I don't necessarily agree with your assessment.

while keeping Tokio a purely private dependency (i.e., not requiring the app to know about or initialize a Tokio event loop).

libs could obviously explicitly start the global reactor as well, making tokio invisible to the end user.

I could be convinced that implicitly starting the global event loop is the right option, but doing so would require answers to the questions I listed. If the right venue to hash those out is the RFC & up front design, then we can do that. I just thought that maybe, if the impl was provided in a PR, it would resolve the questions in my mind.

@aturon
Copy link
Contributor

aturon commented Dec 6, 2017

@carllerche OK cool -- sounds like we agree on some core constraints:

  • It should be possible to embed Tokio within sync APIs without imposing setup burden on the resulting apps.
  • It should be possible to reliably avoid any automatic event loop construction.

You're also suggesting the following additional requirements:

  • It should be possible to control the default event loop thread, including shutdown and restart.

I'm a little iffy on this constraint -- personally, I'm happy with the story that you can either use the default loop which runs forever once started, or else you fully manage it yourself. Can you spell out your thinking here a bit more?

I didn't mean to lawyer about the RFC and whatnot. We can just use this PR to hash out these questions.

@aturon
Copy link
Contributor

aturon commented Dec 6, 2017

Talked with @carllerche on IRC, and we came up with one plausible approach (which I think is a small delta over this PR):

  • There is always a "fallback reactor" that is used when not in the context of a make_default_for.
  • Normally, this fallback reactor is a lazily-started, dedicated event loop thread. Once started, it cannot be shut down.
  • However, we provide an additional API to change the fallback reactor. You can do this to easily ensure that the dedicated thread is never created, and hence give yourself complete control over the reactor and its lifetime.

You can think of the fallback reactor as a global, one-shot, threadsafe Option. It starts as None, and can transition to Some(handle) exactly once. If you invoke something that needs a reactor and the fallback is None, the default one is spun up and the fallback is set to Some. Alternatively, you can set it to Some(your_handle) manually. But once it's set, any further attempt to manually set it will panic.

@alexcrichton would you mind making a commit adding this fallback reactor concept? @carllerche believes this is a plausible approach, but seeing the impl would probably help.

}

impl Default for Handle {
fn default() -> Handle {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the default handle point to the global reactor? For new users this might be bit confusing, and for users not using a global event loop this might point to the wrong event loop.

Would impl Handle { fn global() -> Handle } (so we can call Handle::global) be a better approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this was debated at some length on the RFC, but eventually this may not actually return a global handle in the sense that you'll be able to override the return value of Handle::default within a particular program scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

This commit starts to add support for a global event loop by adding a
`Handle::default` method and implementing it. Currently the support is quite
rudimentary and doesn't support features such as shutdown, overriding the return
value of `Handle::default`, etc. Those will come as future commits.
@alexcrichton
Copy link
Contributor Author

@aturon that sounds reasonable!

I've pushed up a commit which adds a Handle::set_fallback function, is that along the lines of what y'all were thinking?

@aturon
Copy link
Contributor

aturon commented Dec 6, 2017

@alexcrichton Yep, that is precisely what I had in mind!

@carllerche
Copy link
Member

Ok, this looks like a good step to me. Two questions that don't necessarily have to be resolved this PR:

  1. Should set_fallback be a fn on Reactor? Having it on Handle initially felt odd to me, but I think that I could go either way.

  2. How to shutdown the global reactor? The only (remaining) reason that I think it should be shutdown is to avoid FD leaks (the epoll FD never getting closed) which would be raised by linting tools and would be pretty annoying if there was no way to avoid. There doesn't need to be a way to restart it once it is shutdown and I think it would be pretty trivial to have a fn Reactor::shutdown_default_global or something that signals global::HelperThread to terminate and blocks until it does so.

However, as I said, this can be moved to an issue and tracked separately as I don't think it is a core aspect of the "default reactor" design.

Feel free to address or merge, either way.

@carllerche
Copy link
Member

Ooops, messed up the merge, but it is here: 32f2750

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.

None yet

4 participants