Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add flush method on Drain trait #332

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gabyx
Copy link

@gabyx gabyx commented Mar 16, 2024

Closes: slog-rs/async#35
This needs to be thoroughly reviewed, not sure if its good enough =)

  • The return type io:Result<()> could not be turned into result::Result<Self::Ok, Self::Err> since that would need some more redirection or, or I use result::Result<Self::Ok, Self::Err> and return Ok(Self::Ok::new()) if possible in the default flush() implementation.

Make sure to:

  • Add an entry to CHANGELOG.md (if necessary)

@gabyx gabyx changed the title fix: Add flush method on Drain trait feat: Add flush method on Drain trait Mar 16, 2024
@Techcable
Copy link
Member

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
Copy link
Member

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
Copy link
Author

gabyx commented Mar 23, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

question: How to flush on a async logger?
2 participants