Skip to content
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

Refactor: Replace async Mutex for RaftInner.core_state with standard Mutex and a watch channel #1211

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Jul 30, 2024

Changelog

Refactor: Replace async Mutex for RaftInner.core_state with standard Mutex and a watch channel

In this commit, when joining the RaftCore task, the core_state is
first switched from Running to Joining. The thread then blocks
while awaiting the completion of the RaftCore task. Any other threads
observing the Joining state will wait for the first thread to finish
by monitoring a watch channel created by the initial thread.

Refactor: RaftInner::tx_shutdown changed to use std Mutex

RaftInner::tx_shutdown has been changed to use a standard
(synchronous) Mutex instead of an asynchronous one. This change is made
because tx_shutdown does not span across .await points, making the
asynchronous capabilities unnecessary.


This change is Reviewable

`RaftInner::tx_shutdown` has been changed to use a standard
(synchronous) Mutex instead of an asynchronous one. This change is made
because `tx_shutdown` does not span across `.await` points, making the
asynchronous capabilities unnecessary.
schreter
schreter previously approved these changes Jul 30, 2024
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)


openraft/src/raft/core_state.rs line 14 at r1 (raw file):

    Running(JoinHandleOf<C, Result<Infallible, Fatal<C>>>),

    Joining(WatchReceiverOf<C, bool>),

Maybe add a doc comment?


openraft/src/raft/raft_inner.rs line 227 at r1 (raw file):

            Err(mut rx) => {
                // Other thread is waiting for the core to finish.
                let _ = rx.changed().await;

Though this should be sufficient (channel can be cloned only while it's definitely set to false), don't we need to wait for update to true instead just for the change? What about spurious wakeups?

…d Mutex and a watch channel

In this commit, when joining the `RaftCore` task, the `core_state` is
first switched from `Running` to `Joining`. The thread then blocks
while awaiting the completion of the `RaftCore` task. Any other threads
observing the `Joining` state will wait for the first thread to finish
by monitoring a `watch` channel created by the initial thread.
@drmingdrmer drmingdrmer merged commit 5c82dfd into databendlabs:main Jul 30, 2024
30 of 31 checks passed
@drmingdrmer drmingdrmer deleted the 119-join-state branch July 30, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants