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

Move slow db interactions to tokio blocking pool #905

Open
lutter opened this issue May 2, 2019 · 4 comments
Open

Move slow db interactions to tokio blocking pool #905

lutter opened this issue May 2, 2019 · 4 comments
Labels

Comments

@lutter
Copy link
Collaborator

lutter commented May 2, 2019

All database interactions going through Diesel are synchronous, even though most of them happen within tokio's cooperative multitasking framework. That means that long-running db interactions will block the tokio worker they are running on; if enough workers (by default, one per CPU core) block, the whole system grinds to a halt. At that point, no work can be done, including work that doesn't even involve the database, but simply gets scheduled on the tokio runtime.

Upstream Diesel is not going to add async database interactions anytime soon so we need to come up with our own approach. One possible way to address this is with tokio's tokio_threadpool::blocking where slow work gets pushed to a separate pool of slow tasks, freeing up the main workers.

Open questions around this:

  • how to best integrate that and how to change our internal API's (should most Store functions return Future?)
  • how to determine which db work is slow and needs to be treated that way (might be that we should do all db work on the blocking pool)
@lutter
Copy link
Collaborator Author

lutter commented Mar 4, 2020

What we should do is change the store to move actual work to the blocking pool itself by having some internal function Store.with_connection(f: Fn(Connection) -> Result) -> impl Future<Result> which first acquires a semaphore sized according to the max number of connections in the pool and then executes f on the blocking pool. We'd then change all Store methods that right now just get a connection to use with_connection and do their work inside that.

Once we do that, we should also remove the semaphore introduced in #1522

@leoyvens
Copy link
Collaborator

leoyvens commented Apr 5, 2021

We have with_conn now so the API question is solved. Maybe we don't use it everywhere we should but keeping this issue open isn't what's going to solve that.

@leoyvens leoyvens closed this as completed Apr 5, 2021
@leoyvens
Copy link
Collaborator

leoyvens commented Apr 6, 2021

Actually we have way too much spawn_blocking around our code to consider this done. This will be done when graph::task_spawn::spawn_blocking is no longer in use, since it assumes that a future may block.

@neysofu
Copy link
Member

neysofu commented Sep 14, 2022

cc @mangas because we discussed this during yesterday's standup.

@mangas mangas self-assigned this Dec 6, 2022
@fordN fordN added the Stale label Apr 9, 2024
@mangas mangas removed their assignment Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants