spinlock crash

[halayli]

I hit a segfault that was pointing to the static SpinLockMutex variable.

I am not sure it's a good idea to implement a SpinLockMutex in a project like this one. Aside from getting it right on various architectures, the chances of getting it right aren't on your side.

Looking at try_lock implementation:

  bool try_lock() noexcept  { 
    return !flag_.load(std::memory_order_relaxed) && !flag_.exchange(true, std::memory_order_acquire); 
}

It looks suspicious to me, and as far as I can tell by looking at it, is that !flag_.load(std::memory_order_relaxed) should be followed by flag_.compare_exchange_weak(...,std::memory_order_acquire, std::memory_order_relaxed) rather than flag_.exchange()

I suggest to use pthread_spin_lock(3) and friends or a mutex. Just my 2c.

[ThomsonTan]

@halayli which architecture are you getting the spinlock crash? I think memory order should not be an issue for x86/x86_64.

For pthread_spin_lock, one issue we cannot use is it is not cross platform.

[halayli]

@ThomsonTan the crash happened on a c5n.9x aws instance and the code was compiled with -march=skylake-avx512. I'll try to reproduce it tomorrow. Tracer was initialized by a copy/paste from here: https://github.com/open-telemetry/opentelemetry-cpp/blob/main/examples/http/tracer_common.h#L68.

I understand the pthread limitation. It definitely make things more challenging.

[lisr]

It looks suspicious to me, and as far as I can tell by looking at it, is that !flag_.load(std::memory_order_relaxed) should be followed by flag_.compare_exchange_weak(...,std::memory_order_acquire, std::memory_order_relaxed) rather than flag_.exchange()

The usage of flag_.exchange() here looks correct to me, boost also has similar implementation https://github.com/boostorg/smart_ptr/blob/develop/include/boost/smart_ptr/detail/spinlock_std_atomic.hpp#L45. (boot looks very simple on this)

Here is my understanding:
using compare_exchange_weak will NOT set value to true if the expected value is true, but set true if expected is false, exchange() also works on these cases(with slight different behavior that it set true even if expected is true). , since the result value is always true which make no difference on the final decision.