glog icon indicating copy to clipboard operation
glog copied to clipboard

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

Open Nimrod0901 opened this issue 2 years ago • 2 comments

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 avatar Mar 12 '22 12:03 Nimrod0901

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 avatar Mar 12 '22 13:03 Nimrod0901

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)  \

Nimrod0901 avatar Mar 12 '22 13:03 Nimrod0901