Improve handling of backtraces

* Get the library passing `cargo test` again
* Only call `env::var_os` once, cache the result in a global
* Move backtrace-related cfg code to one location, avoids `#[cfg]` traffic
* Don't resolve backtraces until they're displayed

Author: alexcrichton

Cargo.toml

@@ -23,4 +23,4 @@ default = ["backtrace", "example_generated"]
 example_generated = []
 
 [dependencies]
-backtrace = { version = "0.3", optional = true }
+backtrace = { version = "0.3.3", optional = true }

examples/size.rs

@@ -21,18 +21,10 @@ fn main() {
     println!("      ErrorKind::Msg: {}", size_of_val(&msg));
     println!("        String: {}", size_of::<String>());
     println!("    State: {}", size_of::<error_chain::State>());
-    #[cfg(feature = "backtrace")]
-    {
-        let state = error_chain::State {
-            next_error: None,
-            backtrace: None,
-        };
-        println!("      State.next_error: {}", size_of_val(&state.next_error));
-        println!("      State.backtrace: {}", size_of_val(&state.backtrace));
-    }
-    #[cfg(not(feature = "backtrace"))]
-    {
-        let state = error_chain::State { next_error: None };
-        println!("      State.next_error: {}", size_of_val(&state.next_error));
-    }
+    let state = error_chain::State {
+        next_error: None,
+        backtrace: error_chain::InternalBacktrace::new(),
+    };
+    println!("      State.next_error: {}", size_of_val(&state.next_error));
+    println!("      State.backtrace: {}", size_of_val(&state.backtrace));
 }

src/backtrace.rs

@@ -0,0 +1,111 @@
+pub use self::imp::{Backtrace, InternalBacktrace};
+
+#[cfg(feature = "backtrace")]
+mod imp {
+    extern crate backtrace;
+
+    use std::cell::UnsafeCell;
+    use std::env;
+    use std::fmt;
+    use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};
+    use std::sync::{Arc, Mutex};
+
+    /// Internal representation of a backtrace
+    #[doc(hidden)]
+    #[derive(Clone)]
+    pub struct InternalBacktrace {
+        backtrace: Option<Arc<MaybeResolved>>,
+    }
+
+    struct MaybeResolved {
+        resolved: Mutex<bool>,
+        backtrace: UnsafeCell<Backtrace>,
+    }
+
+    unsafe impl Send for MaybeResolved {}
+    unsafe impl Sync for MaybeResolved {}
+
+    pub use self::backtrace::Backtrace;
+
+    impl InternalBacktrace {
+        /// Returns a backtrace of the current call stack if `RUST_BACKTRACE`
+        /// is set to anything but ``0``, and `None` otherwise.  This is used
+        /// in the generated error implementations.
+        #[doc(hidden)]
+        pub fn new() -> InternalBacktrace {
+            static ENABLED: AtomicUsize = ATOMIC_USIZE_INIT;
+
+            match ENABLED.load(Ordering::SeqCst) {
+                0 => {
+                    let enabled = match env::var_os("RUST_BACKTRACE") {
+                        Some(ref val) if val != "0" => true,
+                        _ => false,
+                    };
+                    ENABLED.store(enabled as usize + 1, Ordering::SeqCst);
+                    if !enabled {
+                        return InternalBacktrace { backtrace: None }
+                    }
+                }
+                1 => return InternalBacktrace { backtrace: None },
+                _ => {}
+            }
+
+            InternalBacktrace {
+                backtrace: Some(Arc::new(MaybeResolved {
+                    resolved: Mutex::new(false),
+                    backtrace: UnsafeCell::new(Backtrace::new_unresolved()),
+                })),
+            }
+        }
+
+        /// Acquire the internal backtrace
+        #[doc(hidden)]
+        pub fn as_backtrace(&self) -> Option<&Backtrace> {
+            let bt = match self.backtrace {
+                Some(ref bt) => bt,
+                None => return None,
+            };
+            let mut resolved = bt.resolved.lock().unwrap();
+            unsafe {
+                if !*resolved {
+                    (*bt.backtrace.get()).resolve();
+                    *resolved = true;
+                }
+                Some(&*bt.backtrace.get())
+            }
+        }
+    }
+
+    impl fmt::Debug for InternalBacktrace {
+        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+            f.debug_struct("InternalBacktrace")
+                .field("backtrace", &self.as_backtrace())
+                .finish()
+        }
+    }
+}
+
+#[cfg(not(feature = "backtrace"))]
+mod imp {
+    /// Dummy type used when the `backtrace` feature is disabled.
+    pub type Backtrace = ();
+
+    /// Internal representation of a backtrace
+    #[doc(hidden)]
+    #[derive(Clone, Debug)]
+    pub struct InternalBacktrace {}
+
+    impl InternalBacktrace {
+        /// Returns a new backtrace
+        #[doc(hidden)]
+        pub fn new() -> InternalBacktrace {
+            InternalBacktrace {}
+        }
+
+        /// Returns the internal backtrace
+        #[doc(hidden)]
+        pub fn as_backtrace(&self) -> Option<&Backtrace> {
+            None
+        }
+    }
+}

src/error_chain.rs

@@ -419,40 +419,25 @@ macro_rules! error_chain {
 /// for more details.
 #[macro_export]
 #[doc(hidden)]
-#[cfg(feature = "backtrace")]
 macro_rules! impl_extract_backtrace {
     ($error_name: ident
      $error_kind_name: ident
      $([$link_error_path: path, $(#[$meta_links: meta])*])*) => {
         #[allow(unknown_lints, unused_doc_comment)]
         fn extract_backtrace(e: &(::std::error::Error + Send + 'static))
-            -> Option<::std::sync::Arc<$crate::Backtrace>> {
+            -> Option<$crate::InternalBacktrace> {
             if let Some(e) = e.downcast_ref::<$error_name>() {
-                return e.1.backtrace.clone();
+                return Some(e.1.backtrace.clone());
             }
             $(
                 $( #[$meta_links] )*
                 {
                     if let Some(e) = e.downcast_ref::<$link_error_path>() {
-                        return e.1.backtrace.clone();
+                        return Some(e.1.backtrace.clone());
                     }
                 }
             ) *
             None
         }
     }
 }
-
-/// Macro used to manage the `backtrace` feature.
-///
-/// See
-/// https://www.reddit.com/r/rust/comments/57virt/hey_rustaceans_got_an_easy_question_ask_here/da5r4ti/?context=3
-/// for more details.
-#[macro_export]
-#[doc(hidden)]
-#[cfg(not(feature = "backtrace"))]
-macro_rules! impl_extract_backtrace {
-    ($error_name: ident
-     $error_kind_name: ident
-     $([$link_error_path: path, $(#[$meta_links: meta])*])*) => {}
-}

src/lib.rs

@@ -536,22 +536,10 @@
 //! [`map_err`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.map_err
 //! [`BacktraceFrame`]: https://docs.rs/backtrace/0.3.2/backtrace/struct.BacktraceFrame.html
 
-
-#[cfg(feature = "backtrace")]
-extern crate backtrace;
-
 use std::error;
 use std::iter::Iterator;
-#[cfg(feature = "backtrace")]
-use std::sync::Arc;
 use std::fmt;
 
-#[cfg(feature = "backtrace")]
-pub use backtrace::Backtrace;
-#[cfg(not(feature = "backtrace"))]
-/// Dummy type used when the `backtrace` feature is disabled.
-pub type Backtrace = ();
-
 #[macro_use]
 mod impl_error_chain_kind;
 #[macro_use]
@@ -561,6 +549,10 @@ mod quick_main;
 pub use quick_main::ExitCode;
 #[cfg(feature = "example_generated")]
 pub mod example_generated;
+mod backtrace;
+pub use backtrace::Backtrace;
+#[doc(hidden)]
+pub use backtrace::InternalBacktrace;
 
 #[derive(Debug)]
 /// Iterator over the error chain using the `Error::cause()` method.
@@ -587,38 +579,6 @@ impl<'a> Iterator for Iter<'a> {
     }
 }
 
-/// Returns a backtrace of the current call stack if `RUST_BACKTRACE`
-/// is set to anything but ``0``, and `None` otherwise.  This is used
-/// in the generated error implementations.
-#[cfg(feature = "backtrace")]
-#[doc(hidden)]
-pub fn make_backtrace() -> Option<Arc<Backtrace>> {
-    use std::sync::atomic::{ATOMIC_USIZE_INIT, AtomicUsize, Ordering};
-
-    // The lowest bit indicates whether the value was computed,
-    // while the second lowest bit is the actual "enabled" bit.
-    static BACKTRACE_ENABLED_CACHE: AtomicUsize = ATOMIC_USIZE_INIT;
-
-    let enabled = match BACKTRACE_ENABLED_CACHE.load(Ordering::Relaxed) {
-        0 => {
-            let enabled = match std::env::var_os("RUST_BACKTRACE") {
-                Some(ref val) if val != "0" => true,
-                _ => false
-            };
-            let encoded = ((enabled as usize) << 1) | 1;
-            BACKTRACE_ENABLED_CACHE.store(encoded, Ordering::Relaxed);
-            enabled
-        }
-        encoded => (encoded >> 1) != 0
-    };
-
-    if enabled {
-        Some(Arc::new(Backtrace::new()))
-    } else {
-        None
-    }
-}
-
 /// This trait is implemented on all the errors generated by the `error_chain`
 /// macro.
 pub trait ChainedError: error::Error + Send + 'static {
@@ -662,9 +622,8 @@ pub trait ChainedError: error::Error + Send + 'static {
 
     /// Returns the first known backtrace, either from its State or from one
     /// of the errors from `foreign_links`.
-    #[cfg(feature = "backtrace")]
     #[doc(hidden)]
-    fn extract_backtrace(e: &(error::Error + Send + 'static)) -> Option<Arc<Backtrace>>
+    fn extract_backtrace(e: &(error::Error + Send + 'static)) -> Option<InternalBacktrace>
         where Self: Sized;
 }
 
@@ -698,52 +657,32 @@ pub struct State {
     /// Next error in the error chain.
     pub next_error: Option<Box<error::Error + Send>>,
     /// Backtrace for the current error.
-    #[cfg(feature = "backtrace")]
-    pub backtrace: Option<Arc<Backtrace>>,
+    pub backtrace: InternalBacktrace,
 }
 
 impl Default for State {
-    #[cfg(feature = "backtrace")]
     fn default() -> State {
         State {
             next_error: None,
-            backtrace: make_backtrace(),
+            backtrace: InternalBacktrace::new(),
         }
     }
-
-    #[cfg(not(feature = "backtrace"))]
-    fn default() -> State {
-        State { next_error: None }
-    }
 }
 
 impl State {
     /// Creates a new State type
-    #[cfg(feature = "backtrace")]
     pub fn new<CE: ChainedError>(e: Box<error::Error + Send>) -> State {
-        let backtrace = CE::extract_backtrace(&*e).or_else(make_backtrace);
+        let backtrace = CE::extract_backtrace(&*e)
+            .unwrap_or_else(InternalBacktrace::new);
         State {
             next_error: Some(e),
             backtrace: backtrace,
         }
     }
 
-    /// Creates a new State type
-    #[cfg(not(feature = "backtrace"))]
-    pub fn new<CE: ChainedError>(e: Box<error::Error + Send>) -> State {
-        State { next_error: Some(e) }
-    }
-
-    /// Returns the inner backtrace if present.
-    #[cfg(feature = "backtrace")]
-    pub fn backtrace(&self) -> Option<&Backtrace> {
-        self.backtrace.as_ref().map(|v| &**v)
-    }
-
     /// Returns the inner backtrace if present.
-    #[cfg(not(feature = "backtrace"))]
     pub fn backtrace(&self) -> Option<&Backtrace> {
-        None
+        self.backtrace.as_backtrace()
     }
 }