glog icon indicating copy to clipboard operation
glog copied to clipboard
verified publisher icon google

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

Open Nimrod0901 opened this issue 3 years ago • 4 comments

This PR is to address https://github.com/google/glog/issues/804

Note:

  1. Declare LOG_OCCURRENCES_MOD_N as a local, non-atomic variable and use fetch_add % n to get the exact modulo remainder.
  2. Initialize SOME_KIND_OF_LOG_EVERY_N from 0 to 1 since fetch_add will get the previous value.
  3. Add an extra condition when n == 1.
  4. Introduce a new variable LOG_OCCURRENCES_MOD_N in the macro SOME_KIND_OF_LOG_FIRST_N. Although the name seems unrelated, I prefer not to introduce another macro to name it.
  5. Remove the usage AnnotateBenignRaceSized for variables unlikely to be under a multi-thread situation.

I only fix the atomic version and I have no idea if other versions have a similar problem. Would you mind taking a look? @sergiud @drigz Thanks.

Aside question: I doubt if AnnotateBenignRaceSized will take effect on std::atomic since no data race will happen.

Nimrod0901 avatar Mar 17 '22 15:03 Nimrod0901

Since a fix is not straightforward to get right, I suggest that you add unit tests.

sergiud avatar Mar 18 '22 10:03 sergiud

I make some updates. Please take a look if you have time. The tests I added are most likely to fail on the master branch.

Nimrod0901 avatar Mar 18 '22 12:03 Nimrod0901

I'm very sorry for the delay. I will look at the PR right after 0.6 release.

sergiud avatar Apr 03 '22 14:04 sergiud

I'm very sorry for the delay. I will look at the PR right after 0.6 release.

No problem.

Nimrod0901 avatar Apr 04 '22 03:04 Nimrod0901

I'll close the PR since it's heavily diverged from master. @Nimrod0901 if you want continue working on this please rebase.

sergiud avatar Jun 11 '24 19:06 sergiud