-
Notifications
You must be signed in to change notification settings - Fork 1
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
Remove thread pool, switch to tokio #10
Open
seanlinsley
wants to merge
8
commits into
main
Choose a base branch
from
tokio
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5c14085
switch to tokio runtime
seanlinsley 61a816b
pass struct implementing JobHandler, instead of a closure
seanlinsley 67ccb4b
remove debug message
seanlinsley 6098d0a
Redis: use native TLS
seanlinsley 72a9ff9
cleanup
seanlinsley 26f4531
remove select! block from worker loop
seanlinsley fa6ebe0
there's no need to use block_in_place here
seanlinsley c8b1b01
Merge branch 'main' into tokio
msepga File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,12 @@ | ||
use anyhow::Result; | ||
use async_trait::async_trait; | ||
|
||
use crate::job::Job; | ||
use crate::JobSuccessType; | ||
use crate::JobSuccessType::*; | ||
use crate::errors::{ErrorKind, Result}; | ||
|
||
pub type JobHandlerResult = Result<JobSuccessType>; | ||
|
||
pub trait JobHandler: Send { | ||
fn handle(&mut self, job: &Job) -> JobHandlerResult; | ||
fn cloned(&mut self) -> Box<dyn JobHandler>; | ||
} | ||
|
||
impl<F> JobHandler for F | ||
where F: FnMut(&Job) -> JobHandlerResult + Copy + Send + 'static | ||
{ | ||
fn handle(&mut self, job: &Job) -> JobHandlerResult { | ||
self(job) | ||
} | ||
fn cloned(&mut self) -> Box<dyn JobHandler> { | ||
Box::new(*self) | ||
} | ||
} | ||
|
||
pub fn printer_handler(job: &Job) -> JobHandlerResult { | ||
info!("handling {:?}", job); | ||
Ok(Success) | ||
} | ||
|
||
pub fn error_handler(_: &Job) -> JobHandlerResult { | ||
Err(ErrorKind::JobHandlerError(Box::new("a".parse::<i8>().unwrap_err())).into()) | ||
#[async_trait] | ||
pub trait JobHandler: Send + Sync { | ||
async fn perform(&self, job: &Job) -> JobHandlerResult; | ||
} | ||
|
||
pub fn panic_handler(_: &Job) -> JobHandlerResult { | ||
panic!("yeah, I do it deliberately") | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@msepga has a different approach using a
BoxFuture
. That fixed the lifetime issues I had as of the first commit on this branch, but when integrating it into our own code, the futures generated by other crates caused compile errors because they aren'tSync
.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.
In principle this should be equivalent to
Box<dyn Sync + ...>
given thatJobHandler
requiresSync
here, though we can merge as-is regardless 👍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 what you mean by
JobFuture
-- there isn't a type like that around.My understanding is that the
BoxFuture
version and my attempt in the first commit was running into issues because it required that the returned futures beSync
. This implementation only requires that theJobHandler
isSync
.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.
Typo, should be
JobHandler
in my comment 🙂 So,BoxFuture
doesn't require the future to beSync
, the linked implementation only required the handler closure to beSync
:Here's a playground link that illustrates how the handler can be sync without the returned future being sync. You'll notice that uncommenting line 25 will fail to compile, because the returned future doesn't implement
Sync
, even if the handler is required to do so.We don't have to let this block the merge BTW, I just figured I'd leave a footnote in case it could help clear up how
Box<dyn ... + Fn>
is equivalent to theJobHandler
trait here.