-
Notifications
You must be signed in to change notification settings - Fork 0
Add stop behaviour to GenServer #22
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
Open
juan518munoz
wants to merge
35
commits into
main
Choose a base branch
from
reimpl_cancellation_token
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
35 commits
Select commit
Hold shift + click to select a range
da0dacd
First attempt for stream_listener
ElFantasma c15cd54
add test to stream listener
juan518munoz cfca334
add util fn to convert receiver to stream
juan518munoz 4e3db7f
add bounded channel
juan518munoz de32666
add broadcast listener
juan518munoz 6bfb8a1
fix `spawn_broadcast_listener`
juan518munoz 7a6f79a
unify spawn_listener & remove oneline functions
juan518munoz 4d49882
doc update
juan518munoz 5138e09
add impl of sync spawn listener
juan518munoz 04222ae
rename spawn_listener to spawn_listener_from_iter, and port spawn_lis…
juan518munoz 51ecbb2
add bound channel to threads concurrency
juan518munoz 0a78b64
merge duplicated code
juan518munoz 835bf08
add cancel token with 'flaky' test
juan518munoz 0c24840
unflaky the test
juan518munoz 541cc1e
add cancellation to task impl of spawn_listener
juan518munoz 2950644
docs & clippy
juan518munoz 376deda
Merge branch 'main' into stream_listener
juan518munoz 6f7a305
use futures select inside spawn listener
juan518munoz 7b0e33d
use genserver cancel token on stream
juan518munoz 138890f
add cancelation token to timer
juan518munoz 116241d
add cancellation token to gen server tasks impl
juan518munoz 702e4df
remove bounded channels from tasks impl
juan518munoz 2bed000
remove sync channels from threads impl
juan518munoz ce77a36
deprecate spawn_listener for threads impl
juan518munoz 2dc4cc1
fix imports
juan518munoz 53f5a3e
merge dst branch
juan518munoz ab9f69c
remove impl for threads due to reprecation
juan518munoz 2d6e8db
revert more lines
juan518munoz b5ef429
rename `stop` to `teardown`
juan518munoz 668b41f
refactor teardown logic
juan518munoz 38d6848
merge main
juan518munoz 412f291
remove commented code, reword tracing msg
juan518munoz f37178e
improve default teardown function
juan518munoz e652070
mandatory token cancel
juan518munoz b257707
remove unused variable
juan518munoz 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
This file contains hidden or 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,28 +1,34 @@ | ||
//! GenServer trait and structs to create an abstraction similar to Erlang gen_server. | ||
//! See examples/name_server for a usage example. | ||
use futures::future::FutureExt as _; | ||
use spawned_rt::tasks::{self as rt, mpsc, oneshot}; | ||
use spawned_rt::tasks::{self as rt, mpsc, oneshot, CancellationToken}; | ||
use std::{fmt::Debug, future::Future, panic::AssertUnwindSafe}; | ||
|
||
use crate::error::GenServerError; | ||
|
||
#[derive(Debug)] | ||
pub struct GenServerHandle<G: GenServer + 'static> { | ||
pub tx: mpsc::Sender<GenServerInMsg<G>>, | ||
/// Cancellation token to stop the GenServer | ||
cancellation_token: CancellationToken, | ||
} | ||
|
||
impl<G: GenServer> Clone for GenServerHandle<G> { | ||
fn clone(&self) -> Self { | ||
Self { | ||
tx: self.tx.clone(), | ||
cancellation_token: self.cancellation_token.clone(), | ||
} | ||
} | ||
} | ||
|
||
impl<G: GenServer> GenServerHandle<G> { | ||
pub(crate) fn new(initial_state: G::State) -> Self { | ||
let (tx, mut rx) = mpsc::channel::<GenServerInMsg<G>>(); | ||
let handle = GenServerHandle { tx }; | ||
let cancellation_token = CancellationToken::new(); | ||
let handle = GenServerHandle { | ||
tx, | ||
cancellation_token, | ||
}; | ||
let mut gen_server: G = GenServer::new(); | ||
let handle_clone = handle.clone(); | ||
// Ignore the JoinHandle for now. Maybe we'll use it in the future | ||
|
@@ -40,7 +46,11 @@ impl<G: GenServer> GenServerHandle<G> { | |
|
||
pub(crate) fn new_blocking(initial_state: G::State) -> Self { | ||
let (tx, mut rx) = mpsc::channel::<GenServerInMsg<G>>(); | ||
let handle = GenServerHandle { tx }; | ||
let cancellation_token = CancellationToken::new(); | ||
let handle = GenServerHandle { | ||
tx, | ||
cancellation_token, | ||
}; | ||
let mut gen_server: G = GenServer::new(); | ||
let handle_clone = handle.clone(); | ||
// Ignore the JoinHandle for now. Maybe we'll use it in the future | ||
|
@@ -79,6 +89,10 @@ impl<G: GenServer> GenServerHandle<G> { | |
.send(GenServerInMsg::Cast { message }) | ||
.map_err(|_error| GenServerError::Server) | ||
} | ||
|
||
pub fn cancellation_token(&self) -> CancellationToken { | ||
self.cancellation_token.clone() | ||
} | ||
} | ||
|
||
pub enum GenServerInMsg<G: GenServer> { | ||
|
@@ -168,12 +182,16 @@ where | |
async { | ||
loop { | ||
let (new_state, cont) = self.receive(handle, rx, state).await?; | ||
state = new_state; | ||
if !cont { | ||
break; | ||
} | ||
state = new_state; | ||
} | ||
tracing::trace!("Stopping GenServer"); | ||
handle.cancellation_token().cancel(); | ||
if let Err(err) = self.teardown(handle, state).await { | ||
tracing::error!("Error during teardown: {err:?}"); | ||
} | ||
Ok(()) | ||
} | ||
} | ||
|
@@ -269,6 +287,17 @@ where | |
) -> impl Future<Output = CastResponse<Self>> + Send { | ||
async { CastResponse::Unused } | ||
} | ||
|
||
/// Teardown function. It's called after the stop message is received. | ||
/// It can be overrided on implementations in case final steps are required, | ||
/// like closing streams, stopping timers, etc. | ||
fn teardown( | ||
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. 👏 |
||
&mut self, | ||
_handle: &GenServerHandle<Self>, | ||
_state: Self::State, | ||
) -> impl Future<Output = Result<(), Self::Error>> + Send { | ||
async { Ok(()) } | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
This file contains hidden or 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 hidden or 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 hidden or 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 |
---|---|---|
|
@@ -22,9 +22,16 @@ where | |
{ | ||
let cancellation_token = CancellationToken::new(); | ||
let cloned_token = cancellation_token.clone(); | ||
let gen_server_cancellation_token = handle.cancellation_token(); | ||
let join_handle = rt::spawn(async move { | ||
let _ = select( | ||
// Timer action is ignored if it was either cancelled or the associated GenServer is no longer running. | ||
let cancel_conditions = select( | ||
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. Ah, nice! I like having both, the individual cancellation_token, and the gen_server associated one |
||
Box::pin(cloned_token.cancelled()), | ||
Box::pin(gen_server_cancellation_token.cancelled()), | ||
); | ||
|
||
let _ = select( | ||
cancel_conditions, | ||
Box::pin(async { | ||
rt::sleep(period).await; | ||
let _ = handle.cast(message.clone()).await; | ||
|
@@ -49,10 +56,17 @@ where | |
{ | ||
let cancellation_token = CancellationToken::new(); | ||
let cloned_token = cancellation_token.clone(); | ||
let gen_server_cancellation_token = handle.cancellation_token(); | ||
let join_handle = rt::spawn(async move { | ||
loop { | ||
let result = select( | ||
// Timer action is ignored if it was either cancelled or the associated GenServer is no longer running. | ||
let cancel_conditions = select( | ||
Box::pin(cloned_token.cancelled()), | ||
Box::pin(gen_server_cancellation_token.cancelled()), | ||
); | ||
|
||
let result = select( | ||
Box::pin(cancel_conditions), | ||
Box::pin(async { | ||
rt::sleep(period).await; | ||
let _ = handle.cast(message.clone()).await; | ||
|
This file contains hidden or 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
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.
I think here is the place to
handle.cancellation_token().cancel()
as we shouldn't rely on the implementers to remember cancelling it (and if possible, it shouldn't be public)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.
Good point, considering the
cancellation_token
is part of everyGenServer
, it would be proper that by default all of them call thecancel
function. I have applied this changes inside ofteardown
:https://github.com/lambdaclass/spawned/pull/22/files#diff-932cb8f7a40a4c5e58805fa19b32b0d0fef1afee68560831b86a61d59e0553dcR300
Furthermore, thinking it a bit more, it may be better to have it outside of
teardown
to prevent the user from shooting himself in the foot, as I can't think of any case where it wouldn't want to call thecancel
method.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.
Yes, it should be standard
GenServer
behavior: Stopping it will cancel it's cancellation token always. And we cannot put it in the defaultteardown()
implementation as it can be overridden.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.
Done!