Making async_logger::flush() synchronous and really flush in-memory data.

[walkerlala]

As commented in the PR #2819 , I propose to re-implement the async_logger and make its flush() interface synchronous and really flush in-memory message into file.

The current async_logger::flush() implementation, in my opinion, is very counter-intuitive and broken. It is a long established conventions in the C/C++/POSIX world and database world that a "flush" operation should flush its in-memory cache data into disk , (A "sync", instead, persist the OS file cache into disk). However, the current implemention of flush() just post a flush message into the backend queue and return immediately, without knowing when would the in-memory message flushed to file or whether or not it would be flushed to file (e.g., it might be overwritten). This is just not reasonable, because, if a flush operation is just a undeterministic flush, why would the user call the flush() function explicitly? The user could just let the messages in-queue and flushed by the backend thread periodically or when the queue is full. So, if a user call the flush() explicitly, it is mostly that he/she want to make sure that these messages are onto file after flush.

The current implementation is so counter-intuitive and it is the reason why there are so many issues/discussions about it: #2982 #1696 #2819

I also ran into such a bug while using async_logger::flush()

So, I would propose to change the async_logger::flush() implementation and make it synchronouse.

To make this easier, we should reimplement the async_logger so that it has its own message queue that could be pulled continously by worker threads. This make it easier to sink all the messages belonging to a async_logger.

I would like to propose a patch for this and would like some feedback.

[tt4g]

I see what you mean, but I don't understand the assumption that database or POSIX flushes (fflush(), fsync()) and spdlog flushes (spdlog::logger:flush()) must be equivalent.
This is because spdlog outputs logs to more than just files.

The current workaround would be to access sinks by async_logger::sinks() and call sink->flush() directly.
However, all sinks must be thread-safe.

[tt4g]

As far as I researched, async_logger::flush() was synchronous API until it was changed in 06ffde6.
The exact reason for the change from synchronous to asynchronous API is not clear from the commit message (only @gabime knows that).

IMO.
It is assumed that the purpose of async_logger is to prevent the main logic that should be executed in the application thread (main thread) from being interrupted due to long I/O operations (due to slow disks) when performing log output operations. It then makes sense to change the flash operation to asynchronous API.
If this had not been changed to asynchronous API, it would have been reported that: "slowing down my application by slow async_logger::flush() operations. This is because async_logger::flush() is synchronous API".

[tt4g]

If you are confident it is the right thing to do, why not send a PR?

[walkerlala]

Yeah a pr is pending. I just want to make sure that there is no strong opposition against this idea before sending it.

[tt4g]

I have no opposition to #2819.
If you require async_logger::flush() to be changed to synchronous API, I would oppose it because it breaks API compatibility.

[walkerlala]

I think this does not break API compatibility because flush() was never declared/defined to be async. From your previous quote, in @gabime 's first implementation, flush() was synchronous?

[tt4g]

At first it appears to have been synchronous API, but for the next 8+ years it was asynchronous API.

[gabime]

Since this is an asynchronous interface, flush() is asynchronous as well. However, I do agree that this makes it of limited use, and a blocking flush would indeed be better. Pull requests are welcome.

Btw #2819 seems like a good starting point.

[walkerlala]

What is the difference between 1.x and 2.x branches?

[walkerlala]

#3049