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 thread unsafety in occasional logging when using std::atomic #808

Closed
wants to merge 5 commits into from
Closed

fix thread unsafety in occasional logging when using std::atomic #808

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ba4ad45
fix thread unsafety in occasional logging when using std::atomic
Nimrod0901 Mar 17, 2022
51d5348
replace fetch_add with ++
Nimrod0901 Mar 18, 2022
30f8a84
use unsigned int to avoid UB
Nimrod0901 Mar 18, 2022
821a5b6
add unittest for multiple thread occasional logging for C++11 and later
Nimrod0901 Mar 18, 2022
eeb85a2
replace implicit conversion with explicit one
Nimrod0901 Apr 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 12 additions & 18 deletions src/glog/logging.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1116,44 +1116,38 @@ namespace google {

#if @ac_cv_cxx11_atomic@ && __cplusplus >= 201103L
#define SOME_KIND_OF_LOG_EVERY_N(severity, n, what_to_do) \
static std::atomic<int> LOG_OCCURRENCES(0), LOG_OCCURRENCES_MOD_N(0); \
static std::atomic<int> LOG_OCCURRENCES(1); \
GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \
GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES_MOD_N, sizeof(int), "")); \
++LOG_OCCURRENCES; \
if (++LOG_OCCURRENCES_MOD_N > n) LOG_OCCURRENCES_MOD_N -= n; \
if (LOG_OCCURRENCES_MOD_N == 1) \
int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed) % n; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not clear to me why changing to fetch_add provides a solution here. op++ also calls fetch_add albeit with different memory ordering. Consequently, you cannot avoid the problem you are trying to solve. To be able to, you would need to perform all the involved operations atomically by protecting concurrent access in a critical section.

Copy link
Author

Choose a reason for hiding this comment

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

The problem was "a series of atomic operations of LOG_OCCURRENCES_MOD_N, but in whole, they are not atomic, which unexpected". Here only the LOG_OCCURRENCES is static, the only place we need to protect. Specifically, we only do one atomic operation fetch_add on LOG_OCCURRENCES.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My question is: in what way do the changes fix the problem? The operations are still not atomic as a whole.

Copy link
Author

Choose a reason for hiding this comment

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

The operations are still not atomic as a whole.

Yes. But they don't need to be. There is only one operation that needs to be atomic, access to the static variable LOG_OCCURRENCES.

in what way do the changes fix the problem?

For every use of this macro (under different threads), a uniqueLOG_OCCURRENCES value is assigned, incremented by 1. We use a local variable LOG_OCCURRENCES_MOD_N to get the reminder of % n. By now, there is no need for protection or being atmoic. It can be written like

int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed);
if (LOG_OCCURRENCES_MOD_N % n == 1)

They are the same.

Copy link
Collaborator

@sergiud sergiud Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You cannot do that. Once LOG_OCCURRENCES overflows you will hit undefined behavior.

Also, the change from op++ to fetch_add is not clear to me. Why is this necessary? If you need the previous value, you can use the post increment.

Copy link
Author

Choose a reason for hiding this comment

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

The main idea is to promise every call can have the exact value in the if condition statements to represent if it should be logged. When the static atomic variable is fetch_add to a local variable, the state is recorded. You'll never get the same value.

Copy link
Author

@Nimrod0901 Nimrod0901 Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Once LOG_OCCURRENCES overflows you will hit undefined behavior.

You're right. This may need more consideration.

the change from op++ to fetch_add is not clear to me. Why is this necessary?

int val = op++; // not atomic, there are two operations. self increment and assignment. ++op is the same
int val = op.fetch_add(1) // atomic

Copy link
Collaborator

@sergiud sergiud Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's not correct. With post increment you will get the previous value since it essentially calls fetch_add as well.

Please refer to the documentation of std::atomic::op++ before we continue the discussion. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, It's my bad. You're right. I agree there is no need to use fetch_add. ++ is enough.

Once LOG_OCCURRENCES overflows you will hit undefined behavior.

Use atomic_uint instead. Unsigned integer overflow is not a UB. Will this help?

if (LOG_OCCURRENCES_MOD_N == 1 || n == 1) \
@ac_google_namespace@::LogMessage( \
__FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \
&what_to_do).stream()

#define SOME_KIND_OF_LOG_IF_EVERY_N(severity, condition, n, what_to_do) \
static std::atomic<int> LOG_OCCURRENCES(0), LOG_OCCURRENCES_MOD_N(0); \
static std::atomic<int> LOG_OCCURRENCES(1); \
GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \
GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES_MOD_N, sizeof(int), "")); \
++LOG_OCCURRENCES; \
int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed) % n; \
if ((condition) && \
((LOG_OCCURRENCES_MOD_N=(LOG_OCCURRENCES_MOD_N + 1) % n) == (1 % n))) \
(LOG_OCCURRENCES_MOD_N == 1 || n == 1)) \
@ac_google_namespace@::LogMessage( \
__FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \
&what_to_do).stream()

#define SOME_KIND_OF_PLOG_EVERY_N(severity, n, what_to_do) \
static std::atomic<int> LOG_OCCURRENCES(0), LOG_OCCURRENCES_MOD_N(0); \
static std::atomic<int> LOG_OCCURRENCES(1); \
GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \
GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES_MOD_N, sizeof(int), "")); \
++LOG_OCCURRENCES; \
if (++LOG_OCCURRENCES_MOD_N > n) LOG_OCCURRENCES_MOD_N -= n; \
if (LOG_OCCURRENCES_MOD_N == 1) \
int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed) % n; \
if (LOG_OCCURRENCES_MOD_N == 1 || n == 1) \
@ac_google_namespace@::ErrnoLogMessage( \
__FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \
&what_to_do).stream()

#define SOME_KIND_OF_LOG_FIRST_N(severity, n, what_to_do) \
static std::atomic<int> LOG_OCCURRENCES(0); \
static std::atomic<int> LOG_OCCURRENCES(1); \
GLOG_IFDEF_THREAD_SANITIZER(AnnotateBenignRaceSized(__FILE__, __LINE__, &LOG_OCCURRENCES, sizeof(int), "")); \
if (LOG_OCCURRENCES <= n) \
++LOG_OCCURRENCES; \
if (LOG_OCCURRENCES <= n) \
int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed); \
if (LOG_OCCURRENCES_MOD_N <= n) \
@ac_google_namespace@::LogMessage( \
__FILE__, __LINE__, @ac_google_namespace@::GLOG_ ## severity, LOG_OCCURRENCES, \
&what_to_do).stream()
Expand Down