-
Notifications
You must be signed in to change notification settings - Fork 16
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 support for blocking handlers #65
Changes from 1 commit
09235c7
a13b3aa
328afb9
286b7f3
22b7d7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ use std::sync::{atomic, Arc}; | |
use std::{error::Error as StdError, sync::atomic::AtomicUsize}; | ||
use tokio::io::{AsyncBufRead, AsyncWrite}; | ||
use tokio::net::TcpStream; | ||
use tokio::task::{AbortHandle, JoinSet}; | ||
use tokio::task::{spawn_blocking, AbortHandle, JoinSet}; | ||
|
||
mod builder; | ||
mod health; | ||
|
@@ -20,7 +20,12 @@ pub(crate) const STATUS_RUNNING: usize = 0; | |
pub(crate) const STATUS_QUIET: usize = 1; | ||
pub(crate) const STATUS_TERMINATING: usize = 2; | ||
|
||
type CallbacksRegistry<E> = FnvHashMap<String, runner::BoxedJobRunner<E>>; | ||
pub(crate) enum Callback<E> { | ||
Async(runner::BoxedJobRunner<E>), | ||
Sync(Arc<dyn Fn(Job) -> Result<(), E> + Sync + Send + 'static>), | ||
jonhoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
type CallbacksRegistry<E> = FnvHashMap<String, Callback<E>>; | ||
|
||
/// `Worker` is used to run a worker that processes jobs provided by Faktory. | ||
/// | ||
|
@@ -187,7 +192,16 @@ impl<S: AsyncBufRead + AsyncWrite + Send + Unpin, E: StdError + 'static + Send> | |
.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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's do that in a follow-up (maybe the tech debt one?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pinned in there |
||
Callback::Sync(cb) => { | ||
let cb = Arc::clone(cb); | ||
jonhoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
spawn_blocking(move || cb(job)) | ||
.await | ||
.expect("joined ok") | ||
jonhoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.map_err(Failed::Application) | ||
} | ||
} | ||
} | ||
|
||
async fn report_on_all_workers(&mut self) -> Result<(), Error> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated