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.

Sign up for GitHub

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

Jump to bottom

Fix race conditions in LOG_EVERY_N #492

Merged
merged 5 commits into from
Feb 22, 2021
Merged

Fix race conditions in LOG_EVERY_N #492

merged 5 commits into from
Feb 22, 2021

Conversation

aesophor
Copy link
Contributor

@aesophor aesophor commented Nov 5, 2019 •
edited
Loading

Fixes issue #439 by protecting critical sections with mutexes.

Note:

  1. Use CMakeLists.txt CheckCXXSourceCompiles to check the availability of std::atomic<>
  2. The atomic operations will be handled differently based on the following situations:
    • Compiler supports C++11 - simply declare the integer counters as std::atomic<int>
    • Pre C++11 (MSVC) - InterlockedIncrement(), InterlockedExchangeSubtract()
    • Pre C++11 (GCC) - __sync_add_and_fetch(), __sync_sub_and_fetch()

@sergiud
Copy link
Collaborator

sergiud commented Nov 5, 2019 •
edited
Loading

A mutex is quite heavy. Would thread-local storage possibly suffice?

cc @os12

@os12
Copy link

os12 commented Nov 5, 2019

There is no need for a mutex. These counters just need to become std::atomic (or equivalent non-portable).

👎

@sergiud
Copy link
Collaborator

sergiud commented Nov 5, 2019

Glog still uses C++03. There should be atleast a fallback mechanism for pre C++11 compilers.

os12
os12 reviewed Nov 6, 2019
View reviewed changes
src/glog/logging.h.in Outdated
@@ -897,7 +897,7 @@ PLOG_IF(FATAL, GOOGLE_PREDICT_BRANCH_NOT_TAKEN((invocation) == -1)) \
#define LOG_OCCURRENCES_MOD_N LOG_EVERY_N_VARNAME(occurrences_mod_n_, __LINE__)

#define SOME_KIND_OF_LOG_EVERY_N(severity, n, what_to_do) \
static int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \
thread_local int LOG_OCCURRENCES = 0, LOG_OCCURRENCES_MOD_N = 0; \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong to me. Imagine the same LOG_EVERY_XXX() called from several threads concurrently - every thread will get its own count now...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the counters std::atomic<int>, and now the only thing left is a fallback mechanism for pre C++11 compilers. Do you have any idea?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both GCC and MSVC provide atomic intrinsics. Please see https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html and https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedincrement.

sergiud
sergiud requested changes Nov 6, 2019
View reviewed changes
src/glog/logging.h.in Outdated
// Microsoft Visual Studio 14 (2015) sets __cplusplus==199711 despite
// reasonably good C++11 support, so we set LANG_CXX for it and
// newer versions (_MSC_VER >= 1900).
#define SUPPORT_CPLUSPLUS11 (defined(__GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L || \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::atomic<> availability should be checked in CMakeLists.txt using CheckCXXSymbolExists or CheckCXXSourceCompiles module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@aesophor
fix race conditions in LOG_EVERY_N
997fe7f 
* Use CMakeLists.txt to check the availability of std::atomic<>
* Declare the static integer counters as std::atomic<int> instead of int
if C++11's atomic is available
@aesophor aesophor changed the title src/glog/logging.h.in: fix race conditions in LOG_EVERY_N Fix race conditions in LOG_EVERY_N Nov 6, 2019
@aesophor aesophor requested a review from sergiud November 7, 2019 10:15
@sergiud sergiud self-assigned this Nov 11, 2019
@sergiud
Copy link
Collaborator

sergiud commented Feb 13, 2021

@aesophor Could you please resolve the conflict?

@aesophor
Copy link
Contributor Author

@sergiud I've resolved the conflict and CI has passed successfully. Thanks!

@sergiud sergiud merged commit 9881ea5 into google:master Feb 22, 2021
@sergiud sergiud added this to the 0.5 milestone Mar 30, 2021
@sergiud sergiud mentioned this pull request May 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@os12 os12 os12 left review comments

@sergiud sergiud Awaiting requested review from sergiud

Assignees

@sergiud sergiud

Labels
cla: yes
Projects
None yet
Milestone
0.5
Development

Successfully merging this pull request may close these issues.
4 participants
@aesophor @sergiud @os12 @googlebot