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

Make async_logger::flush() synchronous and wait for the flush to comp… #3049

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

walkerlala
Copy link
Contributor

This patch make the async_logger::flush() interface a synchronous operation, similar to that of normal logger::flush(). The flush operation return only after it succeeds or fail. If failed, the error handler would be invoked with a exception message.

This patch also modify the minimum c++ stdandard to be c++14, to be able to use functions like std::make_unique().

CMakeLists.txt Outdated Show resolved Hide resolved
include/spdlog/details/thread_pool-inl.h Outdated Show resolved Hide resolved
@walkerlala walkerlala force-pushed the make_async_logger_flush_async branch from 7f9d03a to cbf2f32 Compare March 22, 2024 16:12
tests/test_async.cpp Outdated Show resolved Hide resolved
@gabime
Copy link
Owner

gabime commented Mar 22, 2024

It won’t help if the queue was created with discard flag and the flush message gets discarded..

@walkerlala walkerlala force-pushed the make_async_logger_flush_async branch from cbf2f32 to ac79056 Compare March 23, 2024 03:35
@walkerlala
Copy link
Contributor Author

It won’t help if the queue was created with discard flag and the flush message gets discarded..

I don't understand the problem. Could you clarify?

I you mean that the flush signal gets discarded: if the async_msg containing a flush promise gets discard, the promise would be "broken" and then the future.get() call would get and broken promise exception, which would be passed into the error handler for the logger.

@walkerlala walkerlala requested a review from gabime March 23, 2024 09:05
@gabime
Copy link
Owner

gabime commented Mar 23, 2024

Yes, and this means that again flush() won’t work as expected even after this fix. If queue is full, flush() fails without any indication(returns void).

@walkerlala
Copy link
Contributor Author

Yes, and this means that again flush() won’t work as expected even after this fix. If queue is full, flush() fails without any indication(returns void).

If queue is full, flush() will fail, but, there is indication: a std::promise destructed without calling set_value() will cause exception to be thrown. Here is an example:

walkerlala@linuxbox:/tmp
> cat promise.cc
#include <future>
#include <iostream>
#include <stdexcept>

std::future<int> promise_no_set_value() {
    std::promise<int> p;
    std::future<int> f = p.get_future();
    return f;
}

int main() {
    auto f = promise_no_set_value();
    try {
        int res = f.get();
        std::cout << "Got future result: " << res << std::endl;
    } catch (const std::future_error& e) {
        std::cerr << "Caught a future_error: " << e.what() << std::endl;
    } catch (const std::exception& e) {
        std::cout << "exception: " << e.what() << std::endl;
    } catch (...) {
        std::cout << "unknown exception" << std::endl;
    }
}
walkerlala@linuxbox:/tmp
> g++ -Wall -std=c++11 promise.cc -o promise && ./promise
Caught a future_error: std::future_error: Broken promise

So there would be indicatin through the global error handler.

I do noticed the issue that flush() does not return anything. But changing this would change the interface and would break API compatibility, so it might need to go to v2.x. And since spdlog use exception and logger error handler for error propogation, I continue to use this pattern.

As for queue-full-and-drop-message design, I think this is by design and not needed to change. And if we do need to change that, it would not be a easy task because the current design share thread pool and queue between multiple logger.

@walkerlala
Copy link
Contributor Author

By the way, I am seeing "Some checks were not successful" and "1 review requesting changes" but I don't understand what is requested to be changed.

@gabime
Copy link
Owner

gabime commented Mar 23, 2024

So there would be indicatin through the global error handler.

Ok, seems this is the best we can do without breaking the API.
In any case the situation improves after this fix, so I will merge.

@gabime gabime merged commit 6725584 into gabime:v1.x Mar 23, 2024
7 of 8 checks passed
@gabime
Copy link
Owner

gabime commented Mar 23, 2024

Thanks @walkerlala

@gabime
Copy link
Owner

gabime commented Mar 23, 2024

@walkerlala Can you check why msvc test fails here?

@walkerlala
Copy link
Contributor Author

@walkerlala Can you check why msvc test fails here?

Sure. I would check it as soon as possible.

gabime added a commit that referenced this pull request Nov 1, 2024
@gabime gabime mentioned this pull request Nov 1, 2024
gabime added a commit that referenced this pull request Nov 1, 2024
* Revert "Ensure flush callback gets called in move-assign operator (#3232)"

This reverts commit b6da594.

* Revert "Exchange promise for condition_variable when flushing (fixes #3221) (#3228)"

This reverts commit 16e0d2e.

* Revert PR #3049
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.

2 participants