Skip to content

Commit

Permalink
Don't request indirect ping to same addr
Browse files Browse the repository at this point in the history
If a member changed its identity while being probed there was
a chance foca would try to ask the new identity to ping
the old one on its behalf
  • Loading branch information
caio committed Apr 24, 2024
1 parent 8037841 commit bb9ccbb
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 0 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Changelog

## UNRELEASED

- Bugfix: when restarting members, there was a chance foca would
ask a member to ping its own previous identity.
No harm done to the cluster state, but would lead to noise in
the form of `Error::DataFromOurselves` in the member logs
See: https://github.com/caio/foca/issues/34

## v0.17.0 - 2024-03-20

This release contains significant changes aimed at freeing users
Expand Down
39 changes: 39 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,17 @@ where
return Ok(());
}

if !self.members.is_active(&probed_id) {
// Probed member is not active anymore
// Nothing else to be done this probe cycle
#[cfg(feature = "tracing")]
tracing::debug!(
probed_id = tracing::field::debug(&probed_id),
"Probed member isn't active anymore"
);
return Ok(());
}

self.member_buf.clear();
self.members.choose_active_members(
self.config.num_indirect_probes.get(),
Expand Down Expand Up @@ -4246,4 +4257,32 @@ mod tests {
assert!(runtime.is_empty());
}
}

#[test]
fn no_indirect_cycle_if_probed_disappears() {
// ref https://github.com/caio/foca/issues/34

let (mut foca, probed, send_indirect_probe) = craft_probing_foca(1, config());
let mut runtime = AccumulatingRuntime::new();
assert_eq!(1, foca.num_members());

// while `foca` is probing `probed`, it learns
// that it changed its identity for whatever reason
let renewed = probed.bump();
assert_eq!(Ok(()), foca.apply(Member::alive(renewed), &mut runtime));
assert_eq!(1, foca.num_members());
runtime.clear();

// So when the indirect part of the cycle fires
assert_eq!(Ok(()), foca.handle_timer(send_indirect_probe, &mut runtime));

// Since `renewed` and `probed` are technically the same
assert_eq!(renewed.addr(), probed.addr());
assert!(renewed.win_addr_conflict(&probed));
assert!(!probed.win_addr_conflict(&renewed));

// Foca must not request `renewed` to probe `probed`
// on its behalf
assert!(runtime.take_all_data().is_empty());
}
}
6 changes: 6 additions & 0 deletions src/member.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,12 @@ where
self.inner.iter().filter(|m| m.is_active())
}

pub(crate) fn is_active(&self, id: &T) -> bool {
self.inner
.iter()
.any(|member| &member.id == id && member.is_active())
}

pub(crate) fn apply_existing_if<F: Fn(&Member<T>) -> bool>(
&mut self,
mut update: Member<T>,
Expand Down

0 comments on commit bb9ccbb

Please sign in to comment.