diff --git a/src/lib.rs b/src/lib.rs index 8cd1fa86f..29a738de0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -408,7 +408,7 @@ unsafe impl Sync for AtomicUsize {} // the STATE static which determines whether LOGGER has been initialized yet. static mut LOGGER: &dyn Log = &NopLogger; -static STATE: AtomicUsize = AtomicUsize::new(0); +static STATE: AtomicUsize = AtomicUsize::new(UNINITIALIZED); // There are three different states that we care about: the logger's // uninitialized, the logger's initializing (set_logger's been called but @@ -1366,31 +1366,45 @@ fn set_logger_inner(make_logger: F) -> Result<(), SetLoggerError> where F: FnOnce() -> &'static dyn Log, { - let old_state = match STATE.compare_exchange( + // This method doesn't read from `LOGGER` variable at all, + // and if it changes it, it does another store to `STATE` after that. + // We don't need to synchronize CPU caches using memory ordering in this function + // even if another thread set `LOGGER` variable + // because all read accesses to `LOGGER` do that synchronization anyway. + let (Ok(old_state) | Err(old_state)) = STATE.compare_exchange( UNINITIALIZED, INITIALIZING, - Ordering::SeqCst, - Ordering::SeqCst, - ) { - Ok(s) | Err(s) => s, - }; + Ordering::Relaxed, + Ordering::Relaxed, + ); match old_state { UNINITIALIZED => { unsafe { LOGGER = make_logger(); } - STATE.store(INITIALIZED, Ordering::SeqCst); + // Use release ordering here to make write to `LOGGER` static + // and write to any contents of object pointed by `LOGGER` visible + // to other threads after they load `INITIALIZED` `STATE` using Acquire. + STATE.store(INITIALIZED, Ordering::Release); Ok(()) } INITIALIZING => { - while STATE.load(Ordering::SeqCst) == INITIALIZING { - // TODO: replace with `hint::spin_loop` once MSRV is 1.49.0. - #[allow(deprecated)] - std::sync::atomic::spin_loop_hint(); + // Wait until other thread writes a fat reference and a usize + // so wait would be very short so it is OK to use spinlock here. + + // We don't need to use Acquire here because all ways to access + // `LOGGER` from public interface do `Acquire` load before that. + // We need to wait here to ensure that `logger()` function returns + // the valid logger after call. + + while STATE.load(Ordering::Relaxed) == INITIALIZING { + std::hint::spin_loop(); } + Err(SetLoggerError(())) } - _ => Err(SetLoggerError(())), + INITIALIZED => Err(SetLoggerError(())), + unexpected_state => unreachable!("log::STATE has invalid value {}", unexpected_state), } } @@ -1414,17 +1428,18 @@ where /// /// [`set_logger`]: fn.set_logger.html pub unsafe fn set_logger_racy(logger: &'static dyn Log) -> Result<(), SetLoggerError> { - match STATE.load(Ordering::SeqCst) { + match STATE.load(Ordering::Relaxed) { UNINITIALIZED => { LOGGER = logger; - STATE.store(INITIALIZED, Ordering::SeqCst); + STATE.store(INITIALIZED, Ordering::Release); Ok(()) } INITIALIZING => { // This is just plain UB, since we were racing another initialization function unreachable!("set_logger_racy must not be used with other initialization functions") } - _ => Err(SetLoggerError(())), + INITIALIZED => Err(SetLoggerError(())), + unexpected_state => unreachable!("log::STATE has invalid value {}", unexpected_state), } } @@ -1466,7 +1481,8 @@ impl error::Error for ParseLevelError {} /// /// If a logger has not been set, a no-op implementation is returned. pub fn logger() -> &'static dyn Log { - if STATE.load(Ordering::SeqCst) != INITIALIZED { + // Use Acquire to pair it with Release store in `set_logger_inner`. + if STATE.load(Ordering::Acquire) != INITIALIZED { static NOP: NopLogger = NopLogger; &NOP } else {