slog-rs / slog

Structured, contextual, extensible, composable logging for Rust
https://slog.rs/
Apache License 2.0
1.58k stars 95 forks source link

feat: Add `flush` method on `Drain` trait #332

Open gabyx opened 8 months ago

gabyx commented 8 months ago

Closes: https://github.com/slog-rs/async/issues/35 This needs to be thoroughly reviewed, not sure if its good enough =)

Make sure to:

Techcable commented 8 months ago

I think this is an excellent addition! Relying on an explicit mem::drop doesn't always flush correctly in the presence of Logger::clone.

I've wanted this for a while. Thank you for adding it :)

Techcable commented 8 months ago

Right now, the thread:sleep in Async::flush seems surprising to me.

It's specific to the Async logger. If the user switches to another logger like slog-term, it would just flush the underlying .

I think there are two options to make the waiting more explicit. I would appreciate your input on them.

1. Return a FlushStatus

Instead of a function that returns io::Result<()>, it should return io::Result<FlushStatus>:

enum FlushStatus {
    Success,
    QueuedEntries,
}

Then the user can call thread:sleep if they really want to block until success.

2. Pass a explicit FlushWait

enum FlushWait {
    NoWait,
    BlockUntilEmpty(Duration)
}

Then it's more explicit that passing a duration would cause the loop to block. Let me know what you think about these ideas.

gabyx commented 8 months ago

@Techcable: Thanks for the review. Cool if its usable =). I was not yet sure about the function either. Your suggestion sounds very good. Both options are totally legit and better then what I provided!.

The only thing is can we make it such that the signature for an another flush e.g. on term also makes sense? With both approaches you suggest there is still a slight mismatch, either QueuedEntries (term does not have that, and might just return Success anyway), and NoWait makes no sense for term but BlockUntilEmpty(duration) also not because duration is probably obsolete. But I like the type-guarded input! or the output.

Maybe the first one makes more sense, however thinking about what you use in generic code is a Logger where you dont know the underlying thing. So you also do not really care about QueuedEntries. A flush() would be nicest, but does not work for Async, so we need somekind of duration if we block inside. On the other hand if we dont block inside and let the user handle this, its kind of weird that you can call flush and then for some logger its not guaranteed that it happened and you need to run a loop.

Furthermore: What I do not quite understand so far ist: The blocking loop which we do, does that really work? since when you have a producing thread (which produces records fast) and a thread which calls flush at the same time, the loop might never return, because there is never a window where the queue is empty??? Doesn't the queue need something like a DoFlush message (?) which will increment a counting semaphore by 1 (or mutexed integer) which then gets handled by the worker which then decreases the counting semaphore by 1 (until 0), the flush awaits just till the counter reached the level it had before atomically incrementing it, only then we know that all message before that DoFlush message (timestamp wise) are flushed.

  1. Queue: a,b,c,d,e Flush Counter: 0

  2. Some thread 1 requests a flush Queue: Flush(0) a,b,c,d,e Flush Counter: 1

  3. Some more message arrive, some have been handled Queue: k, j, l , Flush(0) Flush Counter: 1

  4. Some thread 2 requests a flush Queue: Flush(1), k, j, l, Flush(0) a,b,c,d,e Flush Counter: 2

  5. Flush(0) is handled == decrement the counter which notifies the thread. Queue: Flush(1), k, j, l Flush Counter: 1

  6. Thread 1 continues because its flush has been handled.

Might that be a solution which makes more sense?