Add support for blocking handlers #65
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
src/worker/builder.rs
Outdated
| /// Note that it is not recommended to mix blocking and non-blocking tasks and so if many of your | ||
| /// handlers are blocking, you will want to launch a dedicated worker process (at least a separate | ||
| /// Tokio Runtime) where only blocking handlers will be used. |
There was a problem hiding this comment.
I'm not sure I expect this to be necessary. As long as we're using spawn_blocking, it should be fine to have some async and some sync handlers.
There was a problem hiding this comment.
This is my reading of this section and namely this passage:
If your code is CPU-bound and you wish to limit the number of threads used to run it, you should use a separate thread pool dedicated to CPU bound tasks. For example, you could consider using the rayon library for CPU-bound tasks. It is also possible to create an extra Tokio runtime dedicated to CPU-bound tasks, but if you do this, you should be careful that the extra runtime runs only CPU-bound tasks, as IO-bound tasks on that runtime will behave poorly.
So "if many handlers are blocking" they can employ one single runtime and mix blocking handlers with async ones, since we are using spawn_blocking (and so are protecting the cooperative scheduling for async tasks), but we are encouraging them to consider launching a dedicated runtime for their blocking tasks. Or you think this note is redundant?
There was a problem hiding this comment.
I think giving a nudge in this direction is fine, although I think the nudge is currently too strong. I wonder if we just want to say something like:
You can mix and match async and blocking handlers in a single
Worker. However, note that there is no active management of the blocking tasks in tokio, and so if you end up with more CPU-intensive blocking handlers executing at the same time than you have cores, the asynchronous handler tasks (and indeed, all tasks) will suffer as a result. If you have a lot of blocking tasks, consider using the standard async job handler and add explicit code to manage the blocking tasks appropriately.
| let handler = self | ||
| .callbacks | ||
| .get(job.kind()) | ||
| .ok_or(Failed::BadJobType(job.kind().to_string()))?; | ||
| handler.run(job).await.map_err(Failed::Application) | ||
| match handler { | ||
| Callback::Async(cb) => cb.run(job).await.map_err(Failed::Application), |
There was a problem hiding this comment.
I am worried that this is not the case for async handlers, panicking in there is not intercepted in the way is now being done for register_blocking_fn. I wonder if might need to use futures::future::FutureExt::catch_unwind for those futures, not sure..
There was a problem hiding this comment.
Let's do that in a follow-up (maybe the tech debt one?)
There was a problem hiding this comment.
pinned in there
Rationale:
We probably should not be discriminating those who have been using the crate for CPU intensive tasks running on the background, e.g. image processing.
We are not adding "fully-fledged"
SyncJobRunnertrait, just to keep things simple.WorkerBuilder::register_blocking_fnshould be considered an escape hatch. AddingSyncJobRunnerlater on - should this be required - will not be a breaking change.This change is