-
Notifications
You must be signed in to change notification settings - Fork 135
refactor(l1): use threads::Genserver for synchronic threaded code #5599
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
base: main
Are you sure you want to change the base?
refactor(l1): use threads::Genserver for synchronic threaded code #5599
Conversation
|
Waiting for upstream changes to be accepted lambdaclass/spawned#64. |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
b02dfab to
04b4e2c
Compare
|
Hey @ElFantasma, I've pinned the revision of the spawned for now. Might be better to wait for the create to be released. |
crates/storage/store.rs
Outdated
| // NOTE: we don't receive `Store` here to avoid cyclic dependencies | ||
| // with the other end of `fkv_ctl` |
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.
Let's move this as a footnote to the bigger comment. With the changes it looks strange.
crates/storage/store.rs
Outdated
| let _ = std::thread::spawn(move || { | ||
| let mut cancellation_token = handle.cancellation_token(); | ||
| loop { |
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 confused, with the new actor shouldn't this be handled by spawned? I thought the goal would be for the worker implementation to only need to worry about the loop body.
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.
Async tasks Genserver uses spawn_listener but here the sync version doesn't have it implemented https://github.com/lambdaclass/spawned/blob/7af72c4ce40eab8b22d33b3302e9da9229319dc0/concurrency/src/threads/stream.rs#L8C1-L17C2 that's why.
Might be a good idea to implement spawn_listener in the upstream. But the function signature of the spawn_listener relies on Stream and the close one I was able to think is IntoInterator. e.g. fn spawn_listener(handle: .., iter: impl IntoIterator<Item = T::CastMsg>) but I was not sure about that change.
Maybe you and @ElFantasma can share some suggestion to implement spawn_listener.
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.
Raised a PR lambdaclass/spawned#66 which should allow to write
fn init(
self,
handle: &spawned_concurrency::threads::GenServerHandle<Self>,
) -> Result<Self, Self::Error> {
let handle = handle.clone();
let rx = self
.trie_upd_rx
.clone()
.into_iter()
.map(|u| Self::CastMsg::TrieUpdates(u));
spawn_listener(handle, rx);
Ok(self)
}
** Motivation **
Block execution is sync code and can benefit from using sync version of
GenServerthat relies on threads.** Changes **
TrieUpdateWorkerand moves thread spawning inside the GenServer::Init.Closes #5565