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

Reduce current thread #717

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

carllerche
Copy link
Member

While exploring the possibility of making the concrete implementation of the current thread executor pluggable, I started to have doubts regarding the daemon task concept and the global function to cancel all running futures.

While at first, they seemed good, I now consider that functionality slightly misplaced. Specifically, both daemon tasks and cancelling running tasks fall within the category of shutdown strategies. IMO the process of shutting down should be controlled by the application and not be something that libraries (future aware libraries, not futures-rs itself) is concerned about.

As such, it didn't seem appropriate for this functionality to be available at a global function level.

I couldn't think of any concrete use case that was best served by a daemon future concept. For example, when using the h2 library, there is a need to spawn "worker" tasks. However, these would never be spawned as a daemon task. Instead, the worker task processes work as it receives it. Work is submitted by "top level" tasks over a handle. The worker task knows when to shutdown by observing that all handles drop.

It could very well be that I am wrong and that the daemon task strategy is correct and all executors should provide this functionality. However, in the effort to remain conservative and actually ship something, I propose:

  1. Punt on figuring out how to make the current thread executor pluggable.
  2. Punt on daemon tasks and a global cancel function.

Both of these can be added at a later time, especially as future releases are planed.

This patch removes the concept of daemon tasks from the current thread
executor as well as the global function to cancel all spawned tasks.

If appropriate, these APIs can be brought back at a later time, but in
an effort to initially be conservative and ship a release, they are
being held back.

The main hesitation being that these APIs are related to shutting down
an executor and do not seem appropriate to be accessible from a global
context. If they remain accessible, then once the executor
implementation is pluggable, it will require that all executors
implement the same logic.

Before making this commitment, an analysis of real use cases should be
done.
@carllerche carllerche changed the base branch from master to tokio-reform January 27, 2018 22:06
@carllerche
Copy link
Member Author

I did leave cancel_all_spawned on &mut Context.

This would make more sense once there is a function on context that can block & drive on a future:

current_thread::run(|ctx| {
    let res = ctx.block_on(my_future);
    ctx.cancel_all_spawned();
    res
})

The eronomics of common patterns can be approved as they are discovered.

@alexcrichton
Copy link
Member

This seems fine to me, @aturon / @cramertj, thoughts?

@carllerche
Copy link
Member Author

I will add Context::block_on to this PR as it seems needed for #698 and #697.

@cramertj
Copy link
Member

Super +1-- this is what I've been angling for in conversations w/ @carllerche and @aturon for a while.

@aturon
Copy link
Member

aturon commented Jan 31, 2018

+1

@carllerche carllerche merged commit 01a4ccf into rust-lang:tokio-reform Jan 31, 2018
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.

4 participants