Dropping AsyncCore in the logger thread will cause panic

[kennytm]

AsyncCore::drop (and AsyncGuard::drop) needs to join the logger thread. When join is called inside the logger thread itself, it will unconditionally panic.

The Drain::log() function is executed in the logger thread. Therefore, if we drop the AsyncCore inside log(), or if log() panics and we have a custom panic hook and we drop the AsyncCore in the hook, it will cause the EDEADLK crash.

Demo:

use slog::{info, o, Discard, Drain, Logger};
use slog_async::Async;
use std::panic::{set_hook, take_hook};
use std::sync::Mutex;
use std::time::Duration;

struct ErrDrain;

impl Drain for ErrDrain {
    type Ok = ();
    type Err = ();

    fn log(&self, _: &slog::Record, _: &slog::OwnedKVList) -> Result<(), ()> {
        Err(())
    }
}

lazy_static::lazy_static! {
    static ref GLOBAL_LOGGER: Mutex<Logger> = {
        let drain = Async::new(ErrDrain.fuse()).thread_name("slogger".to_string()).build().fuse();
        Mutex::new(Logger::root(drain, o!()))
    };
}

fn main() {
    let hook = take_hook();
    set_hook(Box::new(move |pi| {
        hook(pi);
        *GLOBAL_LOGGER.lock().unwrap() = Logger::root(Discard, o!());  // <--- this
    }));

    info!(GLOBAL_LOGGER.lock().unwrap(), "should panic");
    std::thread::sleep(Duration::from_secs(1));
}

The marked line will panic on Linux and macOS.

[dpc]

Yeah. Don't do it. 😁

What are you trying to do exactly? :D

[kennytm]

@dpc As shown in the example there is a global logger for code simplicity.

There is also panic hook is to capture any uncaught panics into the log file before killing the process. During the time between flushing and log and exiting, there might still be some additional logs coming from other threads. We do not want to lose them, and therefore replaced the global logger with the one that logs to stderr.

And that turns out to cause EDEADLK when the panicking thread is the logger thread 🙃.

I'm checking if this issue is the expected behavior before changing the drainer implementation.

[dpc]

Global logger behind a mutex? Oh dear.

Well, dropping AsyncCore will make it wait for the the worker thread to finish ... but how is it going to finish if it's the thread that is waiting? :D

[dpc]

https://docs.rs/slog-async/2.3.0/src/slog_async/lib.rs.html#493

[dpc]

I can't think of any good excuse not to include such "fix". Can you fill a PR? In the meantime I'll think about it a little.