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

LOG_EVERY_N seems not thread safe in C++11 and newer #804

Open
Nimrod0901 opened this issue Mar 12, 2022 · 2 comments
Open

LOG_EVERY_N seems not thread safe in C++11 and newer #804

Nimrod0901 opened this issue Mar 12, 2022 · 2 comments
Labels

Comments

@Nimrod0901
Copy link

Nimrod0901 commented Mar 12, 2022

Consider the code

  static std::atomic<int> LOG_OCCURRENCES(0), LOG_OCCURRENCES_MOD_N(0); \
  ++LOG_OCCURRENCES; \
  if (++LOG_OCCURRENCES_MOD_N > n) LOG_OCCURRENCES_MOD_N -= n; \
  if (LOG_OCCURRENCES_MOD_N == 1) 

Although every statement here is an atomic operation, other threads may fall into the first if condition even though LOG_OCCURRENCES_MOD_N already subtracts n
Simple test

#include <glog/logging.h>
#include <thread>
#include <atomic>

void bar() {
    static std::atomic<int> n(0);
    LOG_EVERY_N(INFO, 2) << ++n;
}
int main() {
    std::vector<std::thread> threads;
    for (size_t i = 0; i < 4000; ++i) {
        std::thread t(&bar);
        threads.emplace_back(std::move(t));
    }
    for (auto& thread : threads) {
        thread.join();
    }
}

Sometimes the output will not be equal to 2000.

I have no idea if it is thread-safe in the older compilers.

@Nimrod0901
Copy link
Author

Nimrod0901 commented Mar 12, 2022

Will this fix?

  static std::atomic<int> LOG_OCCURRENCES(0); \
  int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed); \ // LOG_OCCURRENCES_MOD_N  = ++LOG_OCCURRENCES;
  if (LOG_OCCURRENCES_MOD_N % n == 1)  \

Declare LOG_OCCURRENCES_MOD_N as non-static variable. For every call, LOG_OCCURRENCES_MOD_N will be a different value.

@Nimrod0901
Copy link
Author

LOG_FIRST_N is not thread-safe either.
Simple test

#include <glog/logging.h>
#include <thread>
#include <atomic>

void bar() {
    LOG_FIRST_N(INFO, 1) << "Should be logged exactly once.";
}
int main() {
    std::vector<std::thread> threads;
    for (size_t i = 0; i < 4000; ++i) {
        std::thread t(&bar);
        threads.emplace_back(std::move(t));
    }
    for (auto& thread : threads) {
        thread.join();
    }
}

Sometimes no corresponding message is logged.
Considered fix

  static std::atomic<int> LOG_OCCURRENCES(0); \
  int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed);
  if (LOG_OCCURRENCES_MOD_N <= n)  \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants