search

vthiery / cpp-statsd-client

A header-only StatsD client implemented in C++
MIT License
51 stars 19 forks source link

Memory-visibility correctness of `m_mustExit` #15

Closed jayv closed 3 years ago

jayv commented 3 years ago

Looking at the batching code I noticed m_mustExit being a "plain" bool, which means if I understand the C++ memory model correctly that the compiler/cpu are not required to provide visibility of the changes to other threads so the background batching thread may spin indefinitely.

suggestion: switch m_mustExit to std::atomic_bool and use memory_order_acq_rel on load()/store() [2]

[1] https://github.com/vthiery/cpp-statsd-client/blob/master/include/cpp-statsd-client/UDPSender.hpp#L76 [2] https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering

vthiery commented 3 years ago

Hi @jayv and thank you for reporting this issue.

This bool is indeed problematic. I found a nice SO thread explaining this and checked the explanation in Anthony Williams' book (section 5.3 for the curious ones). I'll prepare the fix today.

vthiery commented 3 years ago

@jayv feel free to have a look at https://github.com/vthiery/cpp-statsd-client/pull/16

vthiery commented 3 years ago

I just released v1.0.2 which contains the fix.