appender: fix WorkerGuard not waiting for writer destruction

[trtt]

Motivation

Can be though of as a continuation to #1120 and #1125.

Example with problematic racy behavior:

use std::io::Write;

struct TestDrop<T: Write>(T);

impl<T: Write> Drop for TestDrop<T> {
    fn drop(&mut self) {
        println!("Dropped");
    }
}

impl<T: Write> Write for TestDrop<T> {
    fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
        self.0.write(buf)
    }
    fn flush(&mut self) -> std::io::Result<()> {
        self.0.flush()
    }
}

fn main() {
    let writer = TestDrop(std::io::stdout());
    let (non_blocking, _guard) = tracing_appender::non_blocking(writer);
    tracing_subscriber::fmt().with_writer(non_blocking).init();
}

Running this test case in a loop with while ./test | grep Dropped; do done, it can be seen that sometimes writer (TestDrop) is not dropped and the message is not printed.
I suppose that proper destruction of non-blocking writer should properly destroy underlying writer.

Solution

Solution involves joining Worker thread (that owns writer) after waiting for it to almost finish avoiding potential deadlock (see #1120 (comment))

[hawkw]

if we get the Disconnected error, this means that the Receiver side of the channel has already been dropped...but that doesn't necessarily mean that the worker thread has terminated yet. should we still call join on the JoinHandle in this case?

[trtt]

if we get the Disconnected error, this means that the Receiver side of the channel has already been dropped...but that doesn't necessarily mean that the worker thread has terminated yet.

right, this can be racy also

If Disconnected can occur only during worker thread (which owns Receiver) destruction, I think it is safe to use join here.
Went ahead and merged Disconnected with Ok case.

[hawkw]

looks good to me overall! i commented on a couple of additional things that might be worth thinking about.

[hawkw]

i'm assuming the flush here was removed because we will already always flush on shutdown in Worker::work()?

[trtt]

that's why I felt safe to remove it

but the actual reason was that it could block (or introduce undefined delay)(#1125 (comment)) when joining the worker. To be honest, I would've just joined the thread, but the IO argument does make sense and I just followed with it.

[trtt]

I realised that within this logic it might be a good idea to drop writer as well: 45da512. There could be some IO there in destructor.