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

Fix stdout_sink_base::log's behavior inconsistency #2646

Merged
merged 3 commits into from
Mar 23, 2023
Merged

Fix stdout_sink_base::log's behavior inconsistency #2646

merged 3 commits into from
Mar 23, 2023

Conversation

25077667
Copy link
Contributor

@25077667 25077667 commented Feb 20, 2023

It will flush every time when it if not defined _WIN32, but not in Windows family.
We viewed the commit 48d4ed9 for fixing issue #1675 . It seems missing this flushing operation in mistake.

It will flush every time when it if not defined _WIN32, but not in
Windows family.
We viewed the commit #48d4ed9 for fixing issue #1675 .
It seems missing this flushing operation in mistake.
@gabime
Copy link
Owner

gabime commented Feb 20, 2023

please use fflush in both cases.

@25077667
Copy link
Contributor Author

@gabime I moved the ffush out of the ifdefine.

@gabime
Copy link
Owner

gabime commented Feb 21, 2023

@25077667 After closer examination I think the original behavior is consistent in both platforms since the windows impl use WriteFile which means no libc memory buffer to be fflushed. Note that fflush only flush the libc buffer. It doesnt call fsync. So I think this PR is not needed.

@gabime gabime closed this Feb 21, 2023
@25077667
Copy link
Contributor Author

@gabime Well, I found this issue from our team's CI.

We used gtest framework to capture the stdout stream. We always could capture the output from SPDLOG_ERROR in Linux and MacOS, but not Windows "sometime".

After I traced code to here, I add std::endl to our code and it fixed the flushing issue.

Honestly speaking, I'm not quite sure what's the behavior after invoke the Window API.
But we knew that the fflush would send data to the kernel.

@gabime
Copy link
Owner

gabime commented Feb 21, 2023

If you are absolutely sure that fflush fixes the issue I don’t mind to add it (and also remove the call to fflush in line #63)

@25077667
Copy link
Contributor Author

@gabime I believe we can keep an eye on things for a week and report back.

@25077667
Copy link
Contributor Author

25077667 commented Mar 1, 2023

Hello @gabime,

It was monitored for a week, and over 40 runs were completed successfully (never trigger any flushing bug issue).
It is a solid remedy, in my opinion.

@25077667
Copy link
Contributor Author

Hello @gabime ,

After a month, we tested over 200 times via our CI.
I think this result could provide a solid remedy.

@gabime
Copy link
Owner

gabime commented Mar 22, 2023

Ok. I reopen the pr. Please remove the the redundant fflush(..) in line 63 since it won't be needed anymore.

@gabime gabime reopened this Mar 22, 2023
@25077667
Copy link
Contributor Author

@gabime ,
Is it possible for you to review my changes?

@gabime gabime merged commit 42d1f40 into gabime:v1.x Mar 23, 2023
@gabime
Copy link
Owner

gabime commented Mar 23, 2023

Merged. Thanks @25077667

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.

3 participants