Skip to content

Commit

Permalink
error: remove Error::Assert, use panic
Browse files Browse the repository at this point in the history
  • Loading branch information
erikgrinaker committed Jun 8, 2024
1 parent bf32044 commit e1c3d9a
Show file tree
Hide file tree
Showing 9 changed files with 49 additions and 73 deletions.
4 changes: 2 additions & 2 deletions src/encoding/keycode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
use de::IntoDeserializer;
use serde::{de, ser};

use crate::errdata;
use crate::error::{Error, Result};
use crate::{errassert, errdata};

// Serializes a key to a binary KeyCode representation.
pub fn serialize<T: serde::Serialize>(key: &T) -> Result<Vec<u8>> {
Expand Down Expand Up @@ -365,7 +365,7 @@ impl<'de, 'a> serde::Deserializer<'de> for &'a mut Deserializer<'de> {
type Error = Error;

fn deserialize_any<V: de::Visitor<'de>>(self, _: V) -> Result<V::Value> {
errassert!("must provide type, KeyCode is not self-describing")
panic!("must provide type, KeyCode is not self-describing")
}

fn deserialize_bool<V: de::Visitor<'de>>(self, visitor: V) -> Result<V::Value> {
Expand Down
28 changes: 3 additions & 25 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@ pub enum Error {
/// The operation was aborted and must be retried. This typically happens
/// with e.g. Raft leader changes.
Abort,
/// An assertion failure. Should never happen.
/// TODO: include backtrace.
Assert(String),
/// Invalid data, typically decoding errors or unexpected internal values.
InvalidData(String),
/// Invalid user input, typically parser or query errors.
Expand All @@ -28,7 +25,6 @@ impl std::fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Error::Abort => write!(f, "operation aborted"),
Error::Assert(msg) => write!(f, "assertion failed: {msg}"),
Error::InvalidData(msg) => write!(f, "invalid data: {msg}"),
Error::InvalidInput(msg) => write!(f, "invalid input: {msg}"),
Error::IO(msg) => write!(f, "io error: {msg}"),
Expand All @@ -48,9 +44,6 @@ impl Error {
match self {
// Aborts don't happen during application, only leader changes.
Error::Abort => true,
// Assertion failures typically indicate invariant violations or
// data corruption, so we should assume it's unsafe to continue.
Error::Assert(_) => false,
// Possible data corruption local to this node.
Error::InvalidData(_) => false,
// Input errors are (likely) deterministic. We could employ command
Expand All @@ -66,12 +59,6 @@ impl Error {
}
}

/// Constructs an Error::Assert via format!() and into().
#[macro_export]
macro_rules! errassert {
($($args:tt)*) => { $crate::error::Error::Assert(format!($($args)*)).into() };
}

/// Constructs an Error::InvalidData via format!() and into().
#[macro_export]
macro_rules! errdata {
Expand All @@ -84,15 +71,6 @@ macro_rules! errinput {
($($args:tt)*) => { $crate::error::Error::InvalidInput(format!($($args)*)).into() };
}

/// Returns an Error::Assert if the given condition is false.
/// TODO: use this instead of assert! where appropriate.
#[macro_export]
macro_rules! asserterr {
($cond:expr, $($args:tt)*) => {
if !$cond { return errassert!($($args)*) }
};
}

/// Result returning Error.
pub type Result<T> = std::result::Result<T, Error>;

Expand Down Expand Up @@ -152,7 +130,7 @@ impl<T> From<crossbeam::channel::TrySendError<T>> for Error {

impl From<hdrhistogram::CreationError> for Error {
fn from(err: hdrhistogram::CreationError) -> Self {
Error::Assert(err.to_string())
panic!("{err}")
}
}

Expand All @@ -170,13 +148,13 @@ impl From<log::ParseLevelError> for Error {

impl From<log::SetLoggerError> for Error {
fn from(err: log::SetLoggerError) -> Self {
Error::Assert(err.to_string())
panic!("{err}")
}
}

impl From<regex::Error> for Error {
fn from(err: regex::Error) -> Self {
Error::Assert(err.to_string())
panic!("{err}")
}
}

Expand Down
33 changes: 16 additions & 17 deletions src/raft/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use super::{NodeID, Term};
use crate::encoding::{self, bincode, Key as _, Value as _};
use crate::error::Result;
use crate::storage;
use crate::{asserterr, errassert};

use serde::{Deserialize, Serialize};

Expand Down Expand Up @@ -148,12 +147,12 @@ impl Log {
/// term does not regress, and that we only vote for one node in a term.
pub fn set_term(&mut self, term: Term, vote: Option<NodeID>) -> Result<()> {
match self.get_term()? {
(t, _) if term < t => return errassert!("term regression {t} → {term}"),
(t, _) if term < t => panic!("term regression {t} → {term}"),
(t, _) if term > t => {} // below, term == t
(0, _) => return errassert!("can't set term 0"),
(0, _) => panic!("can't set term 0"),
(t, v) if t == term && v == vote => return Ok(()),
(_, None) => {}
(_, v) if vote != v => return errassert!("can't change vote {v:?} → {vote:?}"),
(_, v) if vote != v => panic!("can't change vote {v:?} → {vote:?}"),
(_, _) => {}
};
self.engine.set(&Key::TermVote.encode()?, bincode::serialize(&(term, vote))?)?;
Expand All @@ -166,9 +165,9 @@ impl Log {
/// The term must be equal to or greater than the previous entry.
pub fn append(&mut self, term: Term, command: Option<Vec<u8>>) -> Result<Index> {
match self.get(self.last_index)? {
Some(e) if term < e.term => return errassert!("term regression {} → {term}", e.term),
None if self.last_index > 0 => return errassert!("log gap at {}", self.last_index),
None if term == 0 => return errassert!("can't append entry with term 0"),
Some(e) if term < e.term => panic!("term regression {} → {term}", e.term),
None if self.last_index > 0 => panic!("log gap at {}", self.last_index),
None if term == 0 => panic!("can't append entry with term 0"),
Some(_) | None => {}
}
// We could omit the index in the encoded value, since it's also stored
Expand All @@ -186,11 +185,11 @@ impl Log {
pub fn commit(&mut self, index: Index) -> Result<Index> {
let term = match self.get(index)? {
Some(e) if e.index < self.commit_index => {
return errassert!("commit index regression {} → {}", self.commit_index, e.index);
panic!("commit index regression {} → {}", self.commit_index, e.index);
}
Some(e) if e.index == self.commit_index => return Ok(index),
Some(e) => e.term,
None => return errassert!("commit index {index} does not exist"),
None => panic!("commit index {index} does not exist"),
};
self.engine.set(&Key::CommitIndex.encode()?, bincode::serialize(&(index, term))?)?;
// NB: the commit index doesn't need to be fsynced, since the entries
Expand Down Expand Up @@ -251,36 +250,36 @@ impl Log {

// Check that the entries are well-formed.
if first.index == 0 || first.term == 0 {
return errassert!("spliced entry has index or term 0");
panic!("spliced entry has index or term 0");
}
if !entries.windows(2).all(|w| w[0].index + 1 == w[1].index) {
return errassert!("spliced entries are not contiguous");
panic!("spliced entries are not contiguous");
}
if !entries.windows(2).all(|w| w[0].term <= w[1].term) {
return errassert!("spliced entries have term regression");
panic!("spliced entries have term regression");
}

// Check that the entries connect to the existing log (if any), and that the
// term doesn't regress.
match self.get(first.index - 1)? {
Some(base) if first.term < base.term => {
return errassert!("splice term regression {} → {}", base.term, first.term)
panic!("splice term regression {} → {}", base.term, first.term)
}
Some(_) => {}
None if first.index == 1 => {}
None => return errassert!("first index {} must touch existing log", first.index),
None => panic!("first index {} must touch existing log", first.index),
}

// Skip entries that are already in the log.
let mut entries = entries.as_slice();
let mut scan = self.scan(first.index..=last.index)?;
while let Some(entry) = scan.next().transpose()? {
// [0] is ok, because the scan has the same size as entries.
asserterr!(entry.index == entries[0].index, "index mismatch at {entry:?}");
assert!(entry.index == entries[0].index, "index mismatch at {entry:?}");
if entry.term != entries[0].term {
break;
}
asserterr!(entry.command == entries[0].command, "command mismatch at {entry:?}");
assert!(entry.command == entries[0].command, "command mismatch at {entry:?}");
entries = &entries[1..];
}
drop(scan);
Expand All @@ -293,7 +292,7 @@ impl Log {
// Write the entries that weren't already in the log, and remove the
// tail of the old log if any. We can't write below the commit index,
// since these entries must be immutable.
asserterr!(first.index > self.commit_index, "spliced entries below commit index");
assert!(first.index > self.commit_index, "spliced entries below commit index");

for entry in entries {
self.engine.set(&Key::Entry(entry.index).encode()?, entry.encode()?)?;
Expand Down
3 changes: 1 addition & 2 deletions src/raft/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1228,7 +1228,6 @@ fn quorum_value<T: Ord + Copy>(mut values: Vec<T>) -> T {
mod tests {
use super::*;
use crate::encoding::{self, bincode, Value as _};
use crate::errassert;
use crate::raft::{
Entry, Request, RequestID, Response, ELECTION_TIMEOUT_RANGE, HEARTBEAT_INTERVAL,
MAX_APPEND_ENTRIES,
Expand Down Expand Up @@ -1478,7 +1477,7 @@ mod tests {
Ok(node.into())
}
Node::Follower(node) => Ok(node.into_candidate()?.into()),
Node::Leader(node) => errassert!("{} is a leader", node.id),
Node::Leader(node) => panic!("{} is a leader", node.id),
};
for id in ids.iter().copied() {
self.transition(id, campaign, output)?;
Expand Down
6 changes: 3 additions & 3 deletions src/raft/testscripts/log/append
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Appending an entry with term 0 fails, even on an empty log.
!append 0 foo
---
Error: assertion failed: can't append entry with term 0
Panic: can't append entry with term 0

# Appending to an empty log works. The term doesn't have to be 1, and doesn't
# have to match get_term(). The entry is written to the engine and flushed
Expand Down Expand Up @@ -43,8 +43,8 @@ append → 4@5 None
!append 4 old
!append 0
---
Error: assertion failed: term regression 5 → 4
Error: assertion failed: term regression 5 → 0
Panic: term regression 5 → 4
Panic: term regression 5 → 0

# Dump the final status and data.
status
Expand Down
8 changes: 4 additions & 4 deletions src/raft/testscripts/log/commit
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Committing fails on an empty engine.
!commit 1
---
Error: assertion failed: commit index 1 does not exist
Panic: commit index 1 does not exist

# Add some entries.
splice 1@1= 2@1=foo 3@2=bar
Expand All @@ -11,7 +11,7 @@ splice → 3@2 bar
# Committing entry 0 fails.
!commit 0
---
Error: assertion failed: commit index 0 does not exist
Panic: commit index 0 does not exist

# Committing entry 1 works, and updates the commit index.
#
Expand Down Expand Up @@ -50,14 +50,14 @@ last=3@2 commit=3@2
!commit 2
status
---
Error: assertion failed: commit index regression 3 → 2
Panic: commit index regression 3 → 2
last=3@2 commit=3@2

# Committing non-existant indexes error.
!commit 4
status
---
Error: assertion failed: commit index 4 does not exist
Panic: commit index 4 does not exist
last=3@2 commit=3@2

# Dump the raw values.
Expand Down
20 changes: 10 additions & 10 deletions src/raft/testscripts/log/splice
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
# Splicing at index 0 should fail.
!splice 0@1=foo
---
Error: assertion failed: spliced entry has index or term 0
Panic: spliced entry has index or term 0

# Splicing at index 2 should fail (creates gap).
!splice 2@1=foo
---
Error: assertion failed: first index 2 must touch existing log
Panic: first index 2 must touch existing log

# Splicing entries at start should work, both with and without commands, and
# starting at a term after 1. They should be written to the engine and flushed
Expand Down Expand Up @@ -36,27 +36,27 @@ last=2@2 commit=0@0
# Splicing multiple duplicate entries should fail.
!splice 3@2= 3@2=
---
Error: assertion failed: spliced entries are not contiguous
Panic: spliced entries are not contiguous

# Splicing entries with a gap should fail.
!splice 3@2= 5@2=
---
Error: assertion failed: spliced entries are not contiguous
Panic: spliced entries are not contiguous

# Splicing entries with a term regression should fail.
!splice 3@2= 4@1=
---
Error: assertion failed: spliced entries have term regression
Panic: spliced entries have term regression

# Splicing entries with a gap from the base should fail.
!splice 4@2=
---
Error: assertion failed: first index 4 must touch existing log
Panic: first index 4 must touch existing log

# Splicing with a term regression from the base should fail.
!splice 3@1=
---
Error: assertion failed: splice term regression 2 → 1
Panic: splice term regression 2 → 1

# Fully overlapping entries is a noop.
splice 1@2= 2@2=command oplog=true
Expand Down Expand Up @@ -86,7 +86,7 @@ splice → 2@2 command
!splice 2@2=foo
scan
---
Error: assertion failed: command mismatch at Entry { index: 2, term: 2, command: Some([99, 111, 109, 109, 97, 110, 100]) }
Panic: command mismatch at Entry { index: 2, term: 2, command: Some([99, 111, 109, 109, 97, 110, 100]) }
1@2 None
2@2 command

Expand Down Expand Up @@ -180,8 +180,8 @@ last=4@6 commit=2@5
!splice 2@7=
!splice 1@7=
---
Error: assertion failed: spliced entries below commit index
Error: assertion failed: spliced entries below commit index
Panic: spliced entries below commit index
Panic: spliced entries below commit index

# Dump the raw data.
dump
Expand Down
8 changes: 4 additions & 4 deletions src/raft/testscripts/log/term
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ term=0 vote=None
# Storing a 0 term errors.
!set_term 0
---
Error: assertion failed: can't set term 0
Panic: can't set term 0

# set_term stores a term and empty vote, writing it to the engine
# and flushing it to durable storage.
Expand Down Expand Up @@ -46,17 +46,17 @@ term=9 vote=1
# Regressing the term errors.
!set_term 8
---
Error: assertion failed: term regression 9 → 8
Panic: term regression 9 → 8

# Clearing the vote errors.
!set_term 9
---
Error: assertion failed: can't change vote Some(1) → None
Panic: can't change vote Some(1) → None

# Changing the vote errors.
!set_term 9 2
---
Error: assertion failed: can't change vote Some(1) → Some(2)
Panic: can't change vote Some(1) → Some(2)

# The above errors should not have changed the term/vote.
get_term
Expand Down

0 comments on commit e1c3d9a

Please sign in to comment.