Add stop behaviour to GenServer #22
Conversation
| 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.
Ah, nice! I like having both, the individual cancellation_token, and the gen_server associated one
|
Good work! Also, I think that
Does it make sense? |
| if !cont { | ||
| break; | ||
| } | ||
| state = new_state; | ||
| } | ||
| tracing::trace!("Stopping GenServer"); |
There was a problem hiding this comment.
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.
Good point, considering the cancellation_token is part of every GenServer, it would be proper that by default all of them call the cancel function. I have applied this changes inside of teardown:
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 the cancel method.
There was a problem hiding this comment.
Yes, it should be standard GenServer behavior: Stopping it will cancel it's cancellation token always. And we cannot put it in the default teardown() implementation as it can be overridden.
| /// 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( |
Add a
CancellationTokento our Actors.Idea behind is that we can now
stopall the running services working around an actor (timers and listeners as of now) when requested, so they are not left hanging.Note: this PR has no changes to the threads impl due to being no longer maintained, and should only be merged after stream listener PR.
Closes #18