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

Add spawn_pinned to tokio-util #3370

Merged
merged 47 commits into from
Jan 27, 2022

Conversation

AzureMarker
Copy link
Contributor

@AzureMarker AzureMarker commented Jan 3, 2021

Motivation

Sometimes when running a task the program has to handle !Sync data. For example, this could be a database connection (Send + !Sync) which will be passed to other async functions by reference when processing a web server request. Because the connection is !Sync and it is captured as an argument to an async function, the resulting future is !Send. Currently the only way tokio can execute !Send futures is by starting a LocalSet and using spawn_local. This does not work very well when the multi-threaded runtime is used. More context is in #2545.

Solution

This PR introduces tokio_util::task::LocalPoolHandle based on the discussion in #2545 and this PR:

#[derive(Clone)]
pub struct LocalPoolHandle { /* ... */ }

impl LocalPoolHandle {
    pub fn new(pool_size: usize) -> LocalPoolHandle { /* ... */ }

    pub fn spawn_pinned<F, Fut>(&self, create_task: F) -> JoinHandle<Fut::Output>
    where
        F: FnOnce() -> Fut,
        F: Send + 'static,
        Fut: Future + 'static,
        Fut::Output: Send + 'static,
    { /* ... */ }
}

The idea for spawn_pinned is to take in an FnOnce which will create the !Send future. The closure will be sent to a thread which runs a LocalSet on the single-threaded executor. The thread is chosen from a small group of worker threads based on load.

The pinned workers don't shut down when the tokio runtime does, so the
included test never completes.

The worker count is currently hard-coded with a fixme.

PinnedPool does not reset, so if a second runtime is started after the
first finishes, spawn_pinned will not work (the first issue about not
shutting down the workers prevents this scenario from happening atm).
@sfackler
Copy link
Contributor

sfackler commented Jan 4, 2021

It seems like this is a lot of complexity that could be avoided if your database client was Sync. That might be a more reasonable route?

@AzureMarker
Copy link
Contributor Author

AzureMarker commented Jan 4, 2021

I would like to avoid that. The specific DB in my use case is SQLite, but there are other Send + !Sync DB connections (see the connection types used by diesel for example). tokio-postgres gets around this by moving the connection to its own future and passing around handlers to it in the form of Client, but that is not a pattern available everywhere. There are also other situations besides DB connections which require !Send futures.

This is a hard to work around limitation of the multithreaded executor, so I think it's important to provide a general solution instead of only fixing my specific case.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-task Module: tokio/task A-tokio-util Area: The tokio-util crate and removed A-tokio Area: The main tokio crate labels Jan 4, 2021
@MikailBag
Copy link
Contributor

MikailBag commented Jan 4, 2021

Global pool approach is something new for Tokio and Tokio-util.

I'd consider instead:

fn new_local_pool(/*some config, e.g. thread count*/) -> Handle;

struct Handle: Clone;

impl Handle {
    fn spawn(&self, fut) -> JoinHandle;
}

This way user can configure local pool (and use multiple pools if they wish).
Additionally you now have simple shutdown logic: when all Handles are dropped, all worker threads stop.

@AzureMarker
Copy link
Contributor Author

Thanks for the insight and suggestion @MikailBag! I'll rework the code to implement that.

@AzureMarker AzureMarker marked this pull request as ready for review January 5, 2021 19:13
@AzureMarker
Copy link
Contributor Author

@MikailBag Is there anything blocking review of this PR?

@MikailBag
Copy link
Contributor

IMHO your PR looks good.

Please note that I'm not Tokio maintainer though.

@Darksonn
Copy link
Contributor

Darksonn commented Mar 4, 2021

I haven't had time to look closely at this PR yet, sorry.

.gitignore Outdated Show resolved Hide resolved
@Noah-Kennedy
Copy link
Contributor

I'll try and make time to go through this later.

@AzureMarker
Copy link
Contributor Author

@Darksonn I patched up the issues related to task cancellation and task count decrementing. I think it's much more robust now.

tokio-util/src/task/spawn_pinned.rs Outdated Show resolved Hide resolved
tokio-util/src/task/spawn_pinned.rs Outdated Show resolved Hide resolved
tokio-util/src/task/spawn_pinned.rs Outdated Show resolved Hide resolved
tokio-util/src/task/spawn_pinned.rs Outdated Show resolved Hide resolved
tokio-util/src/task/spawn_pinned.rs Outdated Show resolved Hide resolved
tokio-util/tests/spawn_pinned.rs Show resolved Hide resolved
tokio-util/tests/spawn_pinned.rs Outdated Show resolved Hide resolved
tokio-util/tests/spawn_pinned.rs Outdated Show resolved Hide resolved
@AzureMarker
Copy link
Contributor Author

@Darksonn I think this PR is pretty close to being merged. Could you take another look?

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks, it seems good.

@Darksonn
Copy link
Contributor

Sorry about the many delays in reviewing this.

@Darksonn Darksonn merged commit 257053e into tokio-rs:master Jan 27, 2022
@AzureMarker
Copy link
Contributor Author

Thanks for reviewing!

@AzureMarker AzureMarker deleted the feature/spawn-pinned branch January 27, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-task Module: tokio/task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants