Skip to content

Commit

Permalink
Fix: Prevent panic when calling Raft::change_membership() on uninit…
Browse files Browse the repository at this point in the history
…ialized node

Previously, `change_membership()` assumed the current membership config was
always non-empty and used the last config entry. However, uninitialized
nodes lack a membership config, leading to panics.

This commit adds checks to prevent `change_membership()` from panicking
  • Loading branch information
drmingdrmer committed Sep 5, 2024
1 parent 0063194 commit bcc9ac2
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 2 deletions.
2 changes: 1 addition & 1 deletion openraft/src/membership/membership.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ where C: RaftTypeConfig
pub(crate) fn change(mut self, change: ChangeMembers<C>, retain: bool) -> Result<Self, ChangeMembershipError<C>> {
tracing::debug!(change = debug(&change), "{}", func_name!());

let last = self.get_joint_config().last().unwrap().clone();
let last = self.get_joint_config().last().cloned().unwrap_or_default();

let new_membership = match change {
ChangeMembers::AddVoterIds(add_voter_ids) => {
Expand Down
7 changes: 6 additions & 1 deletion openraft/src/quorum/coherent_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ where
if self.is_coherent_with(&other) {
Joint::from(vec![other])
} else {
Joint::from(vec![self.children().last().unwrap().clone(), other])
let last = self.children().last();
if let Some(last) = last {
Joint::from(vec![last.clone(), other])
} else {
Joint::from(vec![other])
}
}
}
}
1 change: 1 addition & 0 deletions tests/tests/membership/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ mod t31_add_remove_follower;
mod t31_remove_leader;
mod t31_removed_follower;
mod t51_remove_unreachable_follower;
mod t52_change_membership_on_uninitialized_node;
mod t99_issue_471_adding_learner_uses_uninit_leader_id;
mod t99_issue_584_replication_state_reverted;
mod t99_new_leader_auto_commit_uniform_config;
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
use std::sync::Arc;

use anyhow::Result;
use maplit::btreemap;
use openraft::ChangeMembers;
use openraft::Config;

use crate::fixtures::ut_harness;
use crate::fixtures::RaftRouter;

/// Call `Raft::change_membership()` on an uninitialized node should not panic due to empty
/// membership.
#[tracing::instrument]
#[test_harness::test(harness = ut_harness)]
async fn change_membership_on_uninitialized_node() -> Result<()> {
let config = Arc::new(
Config {
enable_heartbeat: false,
..Default::default()
}
.validate()?,
);

let mut router = RaftRouter::new(config.clone());
router.new_raft_node(0).await;

let n0 = router.get_raft_handle(&0)?;
let res = n0.change_membership(ChangeMembers::AddVoters(btreemap! {0=>()}), false).await;
tracing::info!("{:?}", res);

let err = res.unwrap_err();
tracing::info!("{}", err);

assert!(err.to_string().contains("forward request to"));

Ok(())
}

0 comments on commit bcc9ac2

Please sign in to comment.