Lock Guard Atomic alternative

Question

I've recently written a Vulkan library for creating 2D applications with ease. The catch was I need std::lock_guard for my window resize event to resize resources related to the window on a secondary render thread (separate from the main GLFW window thread).

While testing on an AMD machine (running on an AMD APU 5600G) std::lock_guard would crash my program. I thought it was strange and tried it again using a blank console program and the same issue happened. Presumably due to issues with memory ordering with caching hits when attempting to lock mutexes on (some?) AMD platforms. The solution was to use std::atomic to force standardized memory ordering.

The end result was my own custom atomic_lock and atomic_mutex. This should work similarly to std::lock_guard except with an escape timeout (in milliseconds):

#pragma once
#ifndef ATOMIC_LOCK
#define ATOMIC_LOCK
    #include <mutex>

    struct atomic_mutex {
    public:
        std::atomic_bool signal;
        std::mutex lock;
    };

    template<bool wait = false, size_t timeout = 100>
    class _NODISCARD_LOCK atomic_lock {
    private:
        std::atomic_bool signal;

    public:
        atomic_mutex& lock;
        
        ~atomic_lock() noexcept { ForceUnlock(); }

        atomic_lock(const atomic_lock&) = delete;

        atomic_lock& operator=(const atomic_lock&) = delete;

        explicit atomic_lock(atomic_mutex& lock) : lock(lock) {
            signal = static_cast<bool>(lock.signal);

            if constexpr(wait) {
                std::chrono::time_point<std::chrono::system_clock> now = std::chrono::system_clock::now();
                std::chrono::milliseconds startTime = duration_cast<std::chrono::milliseconds>(now.time_since_epoch());
                std::chrono::milliseconds endTime = duration_cast<std::chrono::milliseconds>(now.time_since_epoch());
                
                while (signal) {
                    endTime = duration_cast<std::chrono::milliseconds>(now.time_since_epoch());
                    
                    if (endTime.count() - startTime.count() >= static_cast<long long>(timeout))
                        break;
                    
                    signal = static_cast<bool>(lock.signal);
                }
            }

            lock.signal = !signal;
            if (signal == false)
                lock.lock.lock();
        }

        bool AcquiredLock() { return !signal; }

        void ForceUnlock() {
            if (!signal) {
                signal = true;
                lock.signal = false;
                lock.lock.unlock();
            }
        }
    };

#endif

atomic_lock takes in an atomic_mutex (struct with atomic_bool & mutex, see above) and attempts to acquire the mutex before the timeout. Then provides a function with the acquired lock state and another to unlock if needed. Then will unlock itself on destruct (out-of-scope).

Usage Pattern:

atomic_mutex mylock; // default constructors for std::atomic_bool and std::mutex.

void function(atomic_mutex& mutex) {
    atomic_lock alock(mutex);
    // atomic_lock<true, 500> alock(mutex) // template params optional.
    // Do some multi-threaded work...
}

Answer 1

Your standard library is probably fine

std::lock_guard would crash my program.

std::lock_guard is a rather simple class that just calls lock() on the std::mutex you pass it in its constructor, and calls unlock() on it in its destructor. It is almost certainly a programming error if your use of it causes a crash. Possible errors could be that you didn't pass it a valid mutex object, or perhaps it was already locked in the same thread and you try to lock it again.

If calling lock()/unlock() on a mutex would cause a problem, then I would also expect your class to have a problem since you just call those manually.

Presumably due to issues with memory ordering with caching hits when attempting to lock mutexes on (some?) AMD platforms.

This is very unlikely. Even if there is a mistake I would not expect this to cause a crash on lock/unlock, but rather cause a race condition that will rarely trigger.

The interface is not very safe

Constructing an atomic_lock may or may not actually lock the atomic_mutex object. The caller should immediately call AquiredLock() to check whether it was actually locked. Your example usage pattern doesn't even show that. This is in contrast with the default behavior of std::lock_guard, which ensures the mutex is locked.

signal is not updated atomically

Another issue is that while signal is a std::atomic_bool, your class does not read and write lock.signal and call lock.lock() in an atomic way. Consider two threads constructing an atomic_lock object for the same atomic_mutex object at the same time:

Thread 1                        Thread 2
signal = lock.signal; // false
                                signal = lock.signal; // false
lock.signal = !signal; // true
                                lock.signal = !signal; // true
lock.lock.lock();
                                lock.lock.lock();

This causes both threads to immediately lock the mutex, even though it was supposed to be such that one thread would wait for the other to unlock.

atomic_lock::signal should just be a regular bool

I don't think you are supposed to access the same atomic_lock object from multiple threads. Certainly, ForceUnlock() is not thread-safe. That means there is no reason for atomic_lock::signal to be a std::atomic_bool, it could just be a regular bool.

Consider sleeping instead of spinning

The while-loop spins until the mutex is unlocked or some time has elapsed. However, until that happens it uses 100% CPU. This wastes energy for no good reason, and even worse, if you would run this on a computer with only one core, and another thread has locked the mutex, but now your thread has a timeslice, then you can check lock.signal all you want but it will never change until your timeslice ends.

I could tell you to use std::this_thread::yield() or std::this_thread::sleep_for() in your while-loop, but you shouldn't have to do this at all; you should be using std::lock_guard and make sure it doesn't crash.

Also note that std::chrono::system_clock is not the right clock to use; it can suffer from jumps forwards and backwards, for example due to NTP updates, or to daylight savings time kicking in. You should have used std::chrono::steady_clock instead.

Consider using std::timed_mutex

Basically, your class implements a mutex with a timeout. There is already a type for that in the standard library: std::timed_mutex.