Make Error Copy

Author: spacejam

CHANGELOG.md

@@ -23,7 +23,6 @@
 
 ## Breaking Changes
 
-* #1349 The "measure_allocs" feature has been removed.
 * #1135 The "no_metrics" anti-feature has been replaced with
   the "metrics" positive feature.
 * #1178 the `Event` enum has become a unified struct that allows
@@ -52,6 +51,9 @@
 * #1314 `Subscriber::next_timeout` now requires a mutable self
   reference.
 * #1337 Bump MSRV to 1.48.
+* #1349 The "measure_allocs" feature has been removed.
+* `Error` has been modified to be Copy, removing all
+  heap-allocated variants.
 
 
 ## Bug Fixes

src/config.rs

@@ -68,8 +68,7 @@ impl StorageParameters {
                 return Err(Error::Unsupported(
                     "failed to open database that may \
                      have been created using a sled version \
-                     earlier than 0.29"
-                        .to_string(),
+                     earlier than 0.29",
                 ));
             };
             let mut split = line.split(": ").map(String::from);
@@ -282,7 +281,7 @@ impl Inner {
 macro_rules! supported {
     ($cond:expr, $msg:expr) => {
         if !$cond {
-            return Err(Error::Unsupported($msg.to_owned()));
+            return Err(Error::Unsupported($msg));
         }
     };
 }
@@ -550,15 +549,11 @@ impl Config {
                     file.try_lock_exclusive()
                 };
 
-            if let Err(e) = try_lock {
-                return Err(Error::Io(io::Error::new(
+            if try_lock.is_err() {
+                return Err(Error::Io(
                     ErrorKind::Other,
-                    format!(
-                        "could not acquire lock on {:?}: {:?}",
-                        self.db_path().to_string_lossy(),
-                        e
-                    ),
-                )));
+                    "could not acquire database file lock",
+                ));
             }
         }
 
@@ -568,28 +563,27 @@ impl Config {
     fn verify_config(&self) -> Result<()> {
         match self.read_config() {
             Ok(Some(old)) => {
-                supported!(
-                    self.use_compression == old.use_compression,
-                    format!(
-                        "cannot change compression values across restarts. \
-                         old value of use_compression loaded from disk: {}, \
-                         currently set value: {}.",
-                        old.use_compression, self.use_compression,
-                    )
-                );
+                if self.use_compression {
+                    supported!(
+                        old.use_compression,
+                        "cannot change compression configuration across restarts. \
+                        this database was created without compression enabled."
+                    );
+                } else {
+                    supported!(
+                        !old.use_compression,
+                        "cannot change compression configuration across restarts. \
+                        this database was created with compression enabled."
+                    );
+                }
 
                 supported!(
                     self.segment_size == old.segment_size,
-                    format!(
-                        "cannot change the io buffer size across restarts. \
-                         please change it back to {}",
-                        old.segment_size
-                    )
+                    "cannot change the io buffer size across restarts."
                 );
 
-                supported!(
-                    self.version == old.version,
-                    format!(
+                if self.version != old.version {
+                    error!(
                         "This database was created using \
                          pagecache version {}.{}, but our pagecache \
                          version is {}.{}. Please perform an upgrade \
@@ -599,8 +593,13 @@ impl Config {
                         old.version.1,
                         self.version.0,
                         self.version.1,
-                    )
-                );
+                    );
+                    supported!(
+                        self.version == old.version,
+                        "The stored database must use a compatible sled version.
+                        See error log for more details."
+                    );
+                }
                 Ok(())
             }
             Ok(None) => self.write_config(),

src/meta.rs

@@ -54,7 +54,7 @@ where
                     merge_operator: RwLock::new(None),
                 })));
             }
-            Err(Error::CollectionNotFound(_)) => {}
+            Err(Error::CollectionNotFound) => {}
             Err(other) => return Err(other),
         }
 

src/pagecache/mod.rs

@@ -1904,7 +1904,7 @@ impl PageCacheInner {
         if let Some(root) = m.get_root(name) {
             Ok(root)
         } else {
-            Err(Error::CollectionNotFound(name.into()))
+            Err(Error::CollectionNotFound)
         }
     }
 
@@ -2088,10 +2088,10 @@ impl PageCacheInner {
 
         // TODO this feels racy, test it better?
         if let Update::Free = update {
-            Err(Error::ReportableBug(format!(
-                "non-link/replace found in pull of pid {}",
-                pid
-            )))
+            error!("non-link/replace found in pull of pid {}", pid);
+            Err(Error::ReportableBug(
+                "non-link/replace found in pull of page fragments",
+            ))
         } else {
             Ok(update)
         }

src/result.rs

@@ -5,13 +5,7 @@ use std::{
     io,
 };
 
-#[cfg(feature = "testing")]
-use backtrace::Backtrace;
-
-use crate::{
-    pagecache::{DiskPtr, PageView},
-    IVec,
-};
+use crate::pagecache::{DiskPtr, PageView};
 
 /// The top-level result type for dealing with
 /// fallible operations. The errors tend to
@@ -32,27 +26,21 @@ pub(crate) type CasResult<'a, R> =
 
 /// An Error type encapsulating various issues that may come up
 /// in the operation of a `Db`.
-#[derive(Debug)]
+#[derive(Debug, Clone, Copy)]
 pub enum Error {
     /// The underlying collection no longer exists.
-    CollectionNotFound(IVec),
+    CollectionNotFound,
     /// The system has been used in an unsupported way.
-    Unsupported(String),
+    Unsupported(&'static str),
     /// An unexpected bug has happened. Please open an issue on github!
-    ReportableBug(String),
+    ReportableBug(&'static str),
     /// A read or write error has happened when interacting with the file
     /// system.
-    Io(io::Error),
+    Io(io::ErrorKind, &'static str),
     /// Corruption has been detected in the storage file.
     Corruption {
         /// The file location that corrupted data was found at.
         at: Option<DiskPtr>,
-        /// A backtrace for where the corruption was encountered.
-        #[cfg(feature = "testing")]
-        bt: Backtrace,
-        /// A backtrace for where the corruption was encountered.
-        #[cfg(not(feature = "testing"))]
-        bt: (),
     },
     // a failpoint has been triggered for testing purposes
     #[doc(hidden)]
@@ -62,29 +50,7 @@ pub enum Error {
 
 impl Error {
     pub(crate) fn corruption(at: Option<DiskPtr>) -> Error {
-        Error::Corruption {
-            at,
-            #[cfg(feature = "testing")]
-            bt: Backtrace::new(),
-            #[cfg(not(feature = "testing"))]
-            bt: (),
-        }
-    }
-}
-
-impl Clone for Error {
-    fn clone(&self) -> Self {
-        use self::Error::*;
-
-        match self {
-            Io(ioe) => Io(io::Error::new(ioe.kind(), format!("{:?}", ioe))),
-            CollectionNotFound(name) => CollectionNotFound(name.clone()),
-            Unsupported(why) => Unsupported(why.clone()),
-            ReportableBug(what) => ReportableBug(what.clone()),
-            Corruption { at, bt } => Corruption { at: *at, bt: bt.clone() },
-            #[cfg(feature = "failpoints")]
-            FailPoint => FailPoint,
-        }
+        Error::Corruption { at }
     }
 }
 
@@ -95,13 +61,7 @@ impl PartialEq for Error {
         use self::Error::*;
 
         match *self {
-            CollectionNotFound(ref l) => {
-                if let CollectionNotFound(ref r) = *other {
-                    l == r
-                } else {
-                    false
-                }
-            }
+            CollectionNotFound => CollectionNotFound == *other,
             Unsupported(ref l) => {
                 if let Unsupported(ref r) = *other {
                     l == r
@@ -127,15 +87,15 @@ impl PartialEq for Error {
                     false
                 }
             }
-            Io(_) => false,
+            Io(_, _) => false,
         }
     }
 }
 
 impl From<io::Error> for Error {
     #[inline]
     fn from(io_error: io::Error) -> Self {
-        Error::Io(io_error)
+        Error::Io(io_error.kind(), "io error")
     }
 }
 
@@ -144,10 +104,10 @@ impl From<Error> for io::Error {
         use self::Error::*;
         use std::io::ErrorKind;
         match error {
-            Io(ioe) => ioe,
-            CollectionNotFound(name) => io::Error::new(
+            Io(kind, reason) => io::Error::new(kind, reason),
+            CollectionNotFound => io::Error::new(
                 ErrorKind::NotFound,
-                format!("collection not found: {:?}", name),
+                "collection not found"
             ),
             Unsupported(why) => io::Error::new(
                 ErrorKind::InvalidInput,
@@ -180,8 +140,8 @@ impl Display for Error {
         use self::Error::*;
 
         match *self {
-            CollectionNotFound(ref name) => {
-                write!(f, "Collection {:?} does not exist", name,)
+            CollectionNotFound => {
+                write!(f, "Collection does not exist")
             }
             Unsupported(ref e) => write!(f, "Unsupported: {}", e),
             ReportableBug(ref e) => write!(
@@ -192,12 +152,12 @@ impl Display for Error {
             ),
             #[cfg(feature = "failpoints")]
             FailPoint => write!(f, "Fail point has been triggered."),
-            Io(ref e) => write!(f, "IO error: {}", e),
-            Corruption { at, ref bt } => write!(
-                f,
-                "Read corrupted data at file offset {:?} backtrace {:?}",
-                at, bt
-            ),
+            Io(ref kind, ref reason) => {
+                write!(f, "IO error: ({:?}, {})", kind, reason)
+            }
+            Corruption { at } => {
+                write!(f, "Read corrupted data at file offset {:?}", at)
+            }
         }
     }
 }

src/tree.rs

@@ -43,7 +43,6 @@ fn bounds_error() -> Result<()> {
         "Keys and values are limited to \
         128gb on 64-bit platforms and
         512mb on 32-bit platforms."
-            .to_string(),
     ))
 }
 
@@ -938,7 +937,6 @@ impl Tree {
             Err(Error::ReportableBug(
                 "threadpool failed to complete \
                 action before shutdown"
-                    .to_string(),
             ))
         }
     }
@@ -1154,7 +1152,6 @@ impl Tree {
                 "must set a merge operator on this Tree \
                  before calling merge by calling \
                  Tree::set_merge_operator"
-                    .to_owned(),
             ));
         }
 
@@ -1813,7 +1810,7 @@ impl Tree {
 
             if cursor == u64::max_value() {
                 // this collection has been explicitly removed
-                return Err(Error::CollectionNotFound(self.tree_id.clone()));
+                return Err(Error::CollectionNotFound);
             }
 
             let node_opt = self.view_for_pid(cursor, guard)?;

tests/test_tree.rs

@@ -934,9 +934,9 @@ fn tree_subscribers_and_keyspaces() -> Result<()> {
     db.drop_tree(b"1")?;
     db.drop_tree(b"2")?;
 
-    assert_eq!(t1.get(b""), Err(Error::CollectionNotFound(b"1".into())));
+    assert_eq!(t1.get(b""), Err(Error::CollectionNotFound));
 
-    assert_eq!(t2.get(b""), Err(Error::CollectionNotFound(b"2".into())));
+    assert_eq!(t2.get(b""), Err(Error::CollectionNotFound));
 
     let guard = pin();
     guard.flush();