3
\$\begingroup\$

I am using VS 2017 with C++ 17 standard set on Windows OS. What I'm missing in couple of heavily threaded projects is synchronizing mechanism that can be atomically upgraded from shared to exclusive access, without releasing the shared lock. Using boost is not an option for me (another topic is why), so I decided to develop a very light solution by myself. Below implementation works fine in my tests, but I thought I should post it here, cause someone may see or realize something I haven't, or use it in their own projects.

#include <Windows.h>
#include <exception>
#include <atomic>

namespace rip_parallel
{
    // Class upgrade_mutex.
    // Used as a synchronization mutex for upgrade_lock
    class upgrade_mutex
    {
    public:
        // Constructs upgrade_mutex object.
        upgrade_mutex(void) noexcept
            : m_readers(0), m_upgraders(0)
        {
            InitializeSRWLock(&m_sharedlock);

            m_mutex = CreateMutex(nullptr, FALSE, nullptr);

            // We need synchronization event as a barrier that will be set by the owner of shared lock once it needs upgrade.
            m_readevent = CreateEvent(nullptr, TRUE, TRUE, nullptr);
        }
        // Destroys upgrade_mutex object.
        ~upgrade_mutex(void)
        {
            // Once the object is marked for destruction - set the event and close the handle.
            SetEvent(m_readevent);
            CloseHandle(m_readevent);
        }
        // Acquires shared access over the lock. Suspends the calling thread until lock is obtained.
        void lock_shared(void)
        {
            // Request READ access.
            AcquireSRWLockShared(&m_sharedlock);

            // Once acquired, increment readers count.
            m_readers++;
        }
        // Releases shared access over the lock.
        void unlock_shared(void)
        {
            // Release READ access.
            ReleaseSRWLockShared(&m_sharedlock);

            // Once released, decrement readers count.
            m_readers--;
        }
        // Acquires exclusive access over the lock. Suspends the calling thread until lock is obtained.
        void lock(void)
        {
            // Request WRITE access.
            AcquireSRWLockExclusive(&m_sharedlock);
        }
        // Releases exclusive access over the lock.
        void unlock(void)
        {
            // Release WRITE access.
            ReleaseSRWLockExclusive(&m_sharedlock);
        }
        // Waits until shared access over the lock is disabled.
        void wait_read(void)
        {
            // Each thread that wants READ access, has to wait for read to be enabled first.
            // This will enable the thread that wants to acquire upgraded lock to disable further readers while upgrade is active.
            // Writers are not involved in this wait mechanism, cause once at least one thread has shared access, writers are suspended.

            // Wait infinite.
            WaitForSingleObject(m_readevent, INFINITE);
        }
        // Enables shared access over the lock.
        void enable_read(void)
        {
            // Since current thread has upgraded access type, we have to update readers count, since it'll be decremented in unlock_shared.
            m_readers++;

            // We have to keep track of upgraders count, in order to enable read ONLY once all upgarders have completed.
            m_upgraders--;

            if (m_upgraders == 0)
            {
                // Once all upgraders have completed W operation, enable readers.
                SetEvent(m_readevent);
            }
        }
        // Disables shared access over the lock.
        void disable_read(void)
        {
            // The thread that wants to upgrade access, has to disable further read access.
            // It has to reset the event and disable other threads to reach acquiring mutex - otherwise we would deadlock.
            if (m_upgraders == 0)
            {
                // If there are no other upgraders at the moment - reset the event. Otherwise, it's already in non-signaled state.
                ResetEvent(m_readevent);
            }

            // Since current thread is upgrading access type, we have to reduce readers count.
            m_readers--;

            // We have to keep track of upgraders count, in order to enable read ONLY once all upgarders have completed.
            m_upgraders++;
        }
        // Returns active readers count.
        int readers_count(void)
        {
            // Getactual readers count.
            return m_readers;
        }
        // Synchronizes all threads that are requesting upgrade in between, by allowing one writer at a time.
        void upgrade(void)
        {
            // Once we have upgraded our state, we need to acquire exclusive access.
            WaitForSingleObject(m_mutex, INFINITE);
        }
        // Synchronizes all threads that are requesting upgrade in between, by allowing one writer at a time.
        void downgrade(void)
        {
            // Once we have completed exclusive operation we have to release exclusive access.
            ReleaseMutex(m_mutex);
        }
    private:
        SRWLOCK m_sharedlock;
        HANDLE m_mutex;
        HANDLE m_readevent;
        volatile std::atomic<int> m_readers;
        volatile std::atomic<int> m_upgraders;
    };

    enum upgrade_lock_state
    {
        defer_state = 0,
        shared_state = 1,
        exclusive_state = 2,
        upgrade_state = 3
    };

    class upgrade_lock
    {
    public:
        upgrade_lock(upgrade_mutex& ref_mutex, upgrade_lock_state initial_state = defer_state)
            : m_mutex(ref_mutex), m_state(defer_state)
        {
            switch (initial_state)
            {
            case rip_parallel::shared_state:
                lock_shared();
                break;
            case rip_parallel::exclusive_state:
            case rip_parallel::upgrade_state:
                lock_unique();
                break;
            }
        }
        ~upgrade_lock(void)
        {
            unlock();
        }
    public:
        upgrade_lock(const upgrade_lock&) = delete;
        upgrade_lock(upgrade_lock&&) = delete;
    public:
        upgrade_lock& operator=(const upgrade_lock&) = delete;
        upgrade_lock& operator=(upgrade_lock&&) = delete;
    public:
        void unlock(void)
        {
            switch (m_state)
            {
            case rip_parallel::shared_state:
                m_mutex.unlock_shared();
                m_state = defer_state;
                break;
            case rip_parallel::exclusive_state:
                m_mutex.unlock();
                m_state = defer_state;
                break;
            case rip_parallel::upgrade_state:
                m_mutex.downgrade();
                m_mutex.enable_read();

                m_mutex.unlock_shared();
                m_state = defer_state;
                break;
            }
        }
        void lock_unique(void)
        {
            if (m_state == rip_parallel::exclusive_state)
            {
                return;
            }
            if (m_state != rip_parallel::defer_state)
            {
                throw std::exception("While trying to acquire unique lock, invalid state of upgrade_lock found. State was: " + m_state);
            }

            m_mutex.lock();
            m_state = rip_parallel::exclusive_state;
        }
        void lock_shared(void)
        {
            if (m_state == rip_parallel::shared_state)
            {
                return;
            }
            if (m_state != rip_parallel::defer_state)
            {
                throw std::exception("While trying to acquire shared lock, invalid state of upgrade_lock found. State was: " + m_state);
            }

            m_mutex.wait_read();

            m_mutex.lock_shared();
            m_state = rip_parallel::shared_state;
        }
        void lock_upgrade(void)
        {
            if (m_state == upgrade_state)
            {
                return;
            }
            else if (m_state == exclusive_state)
            {
                throw std::exception("While trying to upgrade shared lock, invalid state of upgrade_lock found. State was: " + m_state);
            }
            else if (m_state == defer_state)
            {
                m_mutex.lock_shared();
            }
            m_state = rip_parallel::upgrade_state;

            m_mutex.disable_read();
            while (m_mutex.readers_count())
            {
                Sleep(10);
            }

            m_mutex.upgrade();
            // DO THE JOB
        }
    private:
        upgrade_mutex& m_mutex;
        upgrade_lock_state m_state;
    };
};
// one use case..
using namespace rip_parallel;
upgrade_mutex g_mutex;

#include <chrono>
#include <thread>

void Read(void)
{
    upgrade_lock lock(g_mutex, upgrade_lock_state::shared_state);

    // DO WORK
    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
}

void Write(void)
{
    upgrade_lock lock(g_mutex);
    lock.lock_unique();

    // DO WORK
    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
}

void ReadWrite(void)
{
    upgrade_lock lock(g_mutex, upgrade_lock_state::shared_state);

    // DO SHARED WORK
    std::this_thread::sleep_for(std::chrono::milliseconds(500));

    lock.lock_upgrade();

    // DO EXCLUSIVE WORK
    std::this_thread::sleep_for(std::chrono::milliseconds(500));
}

int main()
{
    std::thread t1(Read);
    std::thread t2(Write);
    std::thread t3(ReadWrite);
    std::thread t4(Read);
    std::thread t5(Read);
    std::thread t6(ReadWrite);
    std::thread t7(Read);
    std::thread t8(ReadWrite);
    std::thread t9(Write);
    std::thread t10(Read);

    t1.join();
    t2.join();
    t3.join();
    t4.join();
    t5.join();
    t6.join();
    t7.join();
    t8.join();
    t9.join();
    t10.join();

    return 0;
}

Once a thread that already owns shared lock wants to upgrade, it needs to reset event (set a barrier) to prevent any future shared lock acquisition. Then it needs to spin/sleep while all current readers release the shared lock. All readers that currently holds the shared lock upon releasing it will decrement readers count and once count reaches zero, that's the moment when all upgraders can proceed. We'll use another mutex to synchronize exclusive access between upgraders - and basically that's it.

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Oct 26 '18 at 11:32
3
\$\begingroup\$

The constructor of upgrade_lock never locks the mutex

When constructing an upgrade_lock variable, the constructor sets m_state = initial_state before calling either lock_shared() or lock_unique(). In the latter two functions, it checks the current value of m_state, and if it already matches the target state, these functions immediately return.

lock_upgrade() is only safe if mutexes are only accessed via upgrade_locks

The function lock_upgrade() has a while loop that waits for readers to become zero. The assumption being that no new read locks can be acquired after the call to disable_read(). However, that only works if the only access to a mutex is via the upgrade_lock() class. If someone calls mutex.lock_shared() directly between the end of the while-loop and before upgrade() is called, then the read lock will be successfully acquired.

Don't use verbs for class and variable names

You named your class upgrade_mutex. This sounds like an action. It is best to use nouns for class and variable names. In this case, you could use upgradable_mutex, or better yet, upgradable_rwlock.

I see you copied Boost's naming convention here, but I would say that in this specific case, they made a bad choice.

Mirror C++11 mutex classes wherever possible

Make your class look and behave existing mutex classes as much as possible. It's the principle of least surprise for the users of your class. For example, C++11 mutexes are held by a lock_guard<>, which again is a noun. Change the class upgrade_lock accordingly, or even better, make a template class lock_guard<> inside the rip_parallel namespace that can work with your locks.

Also, lock_guard<> locks the mutex by default, and this is what users who already know regular mutexes expect. So make the default initial_state = rip_parallel::shared_state.

Make internal functions private or protected

Your class has functions like wait_read() which, I assume, are not meant to be used directly by application code. So they should be hidden. Also, some functions in theupgrade_mutex class, like upgrade(), are misnamed; they don't really upgrade the lock, they only implement part of the functionality needed for upgrading it.

Move as much logic as possible to the mutex class

The upgrade_lock class is there to provide RAII-style locking. The only thing it should do is lock in the constructor, unlock in the destructor, and pass any useful function like upgrading and downgrading to the mutex.

Move constant initialization of member variables out of the constructors

Instead of making the constructor like this:

upgrade_mutex(void): m_readers(0), m_upgraders(0) { ... }

Move the initialization of m_readers and m_upgraders to their declaration:

private:
    volatile std::atomic<int> m_readers = 0;
    volatile std::atomic<int> m_upgraders = 0;

The benefit of this becomes clear when you are writing classes with more than one constructor, or if you have many variables to initialize in your class.

\$\endgroup\$
  • \$\begingroup\$ @g-sliepen very good notes on namings - I'll accept them all. However I'm not sure about your note on implementation not being thread-safe. Have you seen methodvoid upgrade(void) {...} which uses exclusive mutex to synchronize all upgraders? This mutex is used between upgraders only to introduce exclusive access. \$\endgroup\$ – Miloš Ljumović Oct 12 '18 at 12:06
  • 1
    \$\begingroup\$ My apologies, you are right, I must have missed the call to upgrade() or missed that it locked m_mutex. However, I found some other issues, and edited my answer accordingly. \$\endgroup\$ – G. Sliepen Oct 12 '18 at 21:55
  • \$\begingroup\$ @g-sliepen I edited the code and make all adjustments. However your note: 'The constructor of upgrade_lock never locks the mutex' is not quite clear to me. Constructor sets m_state to nothing (defer_state) and if locking mode was specified by the user, in 'switch' operator - function parameter 'initial_state' is used to determine which locking mode and based on that correct locking function is called, which then alters m_state. \$\endgroup\$ – Miloš Ljumović Oct 26 '18 at 11:28
  • \$\begingroup\$ @MilošLjumović - your code simply wrong. 2 different threads can at once own lock in shared and exclusive access. this because you not atomic change state of lock. while one thread acquire lock shared, but yet not do m_readers++ another thread upgrade lock to exclusive. \$\endgroup\$ – RbMm Oct 26 '18 at 12:42
1
\$\begingroup\$

first of all and main - implementation simply incorrect. let some thread call lock_shared() and suspended before m_readers++; line. than another thread also call lock_shared() and than lock_upgrade() - m_mutex.readers_count() will be 0 after disable_read();. as result this thread "got" exclusive access. at once with first thread which already have shared access

your test code nothing prove and useless as result. real test code must create several threads which many thousands times try acquire shared and exclusive lock, change shared to exclusive and visa versa. and on every iteration - check state of lock. you not do this.

about idea of such implementation - really add WaitForSingleObject (so unconditional enter kernel) and busy Sleep() loop - have has a disastrous effect on performance. try do such things simply no sense.

we can note that windows have api for acquire SRW lock in exclusive and shared mode. and exist api for convert exclusive to shared - RtlConvertSRWLockExclusiveToShared. but no api for convert shared to exclusive. really all what need and exist sense in this case

ReleaseSRWLockShared(SRWLock);
AcquireSRWLockExclusive(SRWLock); 

really - convert exclusive to shared - always possible without wait and unlock lock. if exist shared waiters (before exclusive) - api simply unblock it. but in case we want convert shared to exclusive - in general this is impossible without wait - if exist several shared owners. reasonable option in this case simply release lock and wait for exclusive as usual.

also about volatile on std::atomic - this is not need. the std::atomic already volatile by design

\$\endgroup\$
-1
\$\begingroup\$

As @vogel612 suggested updated code is more of an answer rather than edited question. Here is the updated code:

#pragma once
#include <Windows.h>

#include <atomic>
#include <mutex>
#include <exception>

namespace rip_parallel
{
    // Forward declaration needed for 'friend' specification.
    class upgradable_lock;

    // Class upgradable_mutex.
    // Used as a synchronization primitve for upgradable_lock.
    class upgradable_mutex
    {
    public:
        friend class upgradable_lock;
    public:
        // Constructs upgradable_mutex object.
        upgradable_mutex(void) noexcept
        {
            InitializeSRWLock(&m_sharedlock);

            // We need synchronization event as a barrier that will be set by the owner of shared lock once it needs upgrade.
            m_readevent = CreateEvent(nullptr, TRUE, TRUE, nullptr);
        }
        // Destroys upgradable_mutex object.
        ~upgradable_mutex(void)
        {
            // Once the object is marked for destruction - set the event and close the handle.
            SetEvent(m_readevent);
            CloseHandle(m_readevent);
        }
    public:
        // Deleted copy constructor.
        upgradable_mutex(const upgradable_mutex&) = delete;
        // Deleted move constructor.
        upgradable_mutex(upgradable_mutex&&) = delete;
    public:
        // Deleted copy assignment operator.
        upgradable_mutex& operator=(const upgradable_mutex&) = delete;
        // Deleted move assignment operator.
        upgradable_mutex& operator=(upgradable_mutex&&) = delete;
    public:
        // Acquires shared access over the lock.
        void lock_shared(void)
        {
            // Each thread that wants READ access, has to wait for read to be enabled first.
            // This will enable the thread(s) that wants to acquire upgrade lock to disable further reads while upgrade is active.
            // Writers are not involved in this wait mechanism, cause once at least one thread has shared access, all writers are suspended.

            // Wait infinite.
            WaitForSingleObject(m_readevent, INFINITE);

            // Here we need to acquire a READ lock (primary). However we also have to update readers count atomically whilst obtaining the lock.
            // So, we have to use secondary lock (std::unique_lock). Setting the secondary lock (std::unique_lock) while acquiring the primary 
            // lock (SRWLock) will lead to a deadlock if we do not leave a few milliseconds for other threads to complete their operations and 
            // release the primary lock.
            while (true)
            {
                // Scope for the lock.
                {
                    // This lock allows us to do the following in atomic operation:
                    //  1. Acquire the SRWLock
                    //  2. Alter number of readers
                    std::unique_lock<std::mutex> guard(m_mutexguard);

                    // We'll try to acquire READ lock.
                    BOOLEAN bResult = TryAcquireSRWLockShared(&m_sharedlock);
                    if (bResult)
                    {
                        // Once acquired, increment readers count.
                        m_readers++;

                        // Acquired. Exit.
                        break;
                    }

                    // Everything executed inside current scope is atomic.
                }

                // If we cannot acquire it, sleep shortly and try again.
                Sleep(10);
            }
        }
        // Releases shared access over the lock.
        void unlock_shared(void)
        {
            // Release READ access.
            ReleaseSRWLockShared(&m_sharedlock);

            // Protect readers count manipulation.
            std::unique_lock<std::mutex> guard(m_mutexguard);

            // Once released, decrement readers count.
            m_readers--;
        }
        // Acquires exclusive access over the lock. Suspends the calling thread until lock is obtained.
        void lock(void)
        {
            // Since we're not gonna use 2 locks for WRITE access, we won't use TryAcquireSRWLockExclusive to 
            // acquire the WRITE lock. This will make lock obtaining fair and won't be causing starvation of writers.
            AcquireSRWLockExclusive(&m_sharedlock);
        }
        // Releases exclusive access over the lock.
        void unlock(void)
        {
            // Release WRITE access.
            ReleaseSRWLockExclusive(&m_sharedlock);
        }
    private:
        // Enables shared access over the lock.
        void enable_read(void)
        {
            // Scope for the lock.
            {
                // Protect readers count manipulation.
                std::unique_lock<std::mutex> guard(m_mutexguard);

                // Since current thread has upgraded access type, we have to update readers count, since it'll be decremented in unlock_shared.
                m_readers++;
            }

            // We have to keep track of upgraders count, in order to enable READ, once all upgarders have completed.
            m_upgraders--;

            if (m_upgraders == 0)
            {
                // Once all upgraders have completed WRITE operation, enable readers.
                SetEvent(m_readevent);
            }
        }
        // Disables shared access over the lock.
        void disable_read(void)
        {
            // The thread(s) that wants to upgrade access, has to disable further READ access.
            // It means that it has to reset the event and disable other threads to acquire mutex - otherwise we would deadlock.
            if (m_upgraders == 0)
            {
                // If there are no other upgraders at the moment - reset the event. Otherwise, it's already in non-signaled state.
                ResetEvent(m_readevent);
            }

            // Scope for the lock.
            {
                // Protect readers count manipulation.
                std::unique_lock<std::mutex> guard(m_mutexguard);

                // Since current thread is upgrading access type, we have to reduce readers count.
                m_readers--;
            }

            // We have to keep track of upgraders count, in order to enable READ, once all upgarders have completed.
            m_upgraders++;
        }
        // Returns active readers count.
        int readers_count(void)
        {
            // Protect readers count manipulation.
            std::unique_lock<std::mutex> guard(m_mutexguard);

            // Get actual readers count.
            return m_readers;
        }
        // Synchronizes all threads that are requesting upgrade, by allowing one writer at a time.
        void upgrader_acquire_exclusive_access(void)
        {
            // Once we have upgraded our state, we need to acquire exclusive access.
            m_mutex.lock();
        }
        // Synchronizes all threads that are requesting upgrade, by allowing one writer at a time.
        void upgrader_release_exclusive_access(void)
        {
            // Once we have completed exclusive operation we have to release exclusive access.
            m_mutex.unlock();
        }
    private:
        HANDLE m_readevent              = nullptr;
        SRWLOCK m_sharedlock            = SRWLOCK_INIT;
        int m_readers                   = 0;
        std::atomic<int> m_upgraders    = 0;
        std::mutex m_mutexguard;
        std::mutex m_mutex;
    };

    // Enum upgradable_lock_state.
    // Used to determine internal state of upgradable_lock object.
    enum upgradable_lock_state
    {
        // No state. No locking occured.
        defer_state     = 0,
        // Shared state. Lock is obtained in READ mode.
        shared_state    = 1,
        // Exclusive state. Lock is obtained in WRITE mode.
        exclusive_state = 2,
        // Upgraded state. Lock that was previously in READ mode, has upgraded to WRITE mode.
        upgrade_state   = 4
    };

    // Class upgradable_lock.
    // Lightweight READ/WRITE lock abstraction which supports atomical upgrade from shared to exclusive state.
    class upgradable_lock
    {
    public:
        // Constructs upgradable_lock object. Supports RAII-style locking.
        upgradable_lock(upgradable_mutex& ref_mutex, upgradable_lock_state initial_state = rip_parallel::shared_state)
            : m_mutex(ref_mutex)
        {
            // Determine trhe requested lock state.
            switch (initial_state)
            {
            case rip_parallel::shared_state:
                // User has specified shared state.
                lock_shared();
                break;
            case rip_parallel::exclusive_state:
            case rip_parallel::upgrade_state:
                // Whether the user has specified exclusive or upgrade state, at the end we want exclusive access.
                lock_unique();
                break;
            }
        }
        // Automatically releases the lock and destroys the upgradable_lock object.
        ~upgradable_lock(void)
        {
            // Unlock the object.
            unlock();
        }
    public:
        // Deleted copy constructor.
        upgradable_lock(const upgradable_lock&) = delete;
        // Deleted move constructor.
        upgradable_lock(upgradable_lock&&) = delete;
    public:
        // Deleted copy assignment operator.
        upgradable_lock& operator=(const upgradable_lock&) = delete;
        // Deleted move assignment operator.
        upgradable_lock& operator=(upgradable_lock&&) = delete;
    public:
        // Unlocks the upgradable_lock object.
        void unlock(void)
        {
            // We need to determine object state in order to know which unlocking mechanism to execute.
            switch (m_state)
            {
            case rip_parallel::shared_state:
                // If we are in shared state - all we need is to invoke unlock_shared. Mutex will handle the rest.
                m_mutex.unlock_shared();
                break;
            case rip_parallel::exclusive_state:
                // If we are in exclusive state - all we need is to invoke unlock. Mutex will handle the rest.
                m_mutex.unlock();
                break;
            case rip_parallel::upgrade_state:
                // If we were in the upgrade state, then we need to release the upgraders exclusive lock.
                m_mutex.upgrader_release_exclusive_access();
                // Then we need to enable future readers access.
                m_mutex.enable_read();
                // At the end, since upgrader just became another reader, we need to unlock shared access.
                m_mutex.unlock_shared();
                break;
            }
            // In any case above, state has become - 'no lock'.
            m_state = rip_parallel::defer_state;
        }
        // Acquires exclusive access over the lock. Suspends the calling thread until lock is obtained.
        void lock_unique(void)
        {
            // Recursive calls are not supported.
            if (m_state == rip_parallel::exclusive_state)
            {
                return;
            }
            // If we are in any state, other than 'no lock' and 'exclusive' - results are undefined.
            // It's safer to throw exception.
            if (m_state != rip_parallel::defer_state)
            {
                throw std::exception("While trying to acquire unique lock, invalid state of upgradable_lock was found. State was: " + m_state);
            }

            // Obtain the exclusive lock.
            m_mutex.lock();
            // Update the state accordingly.
            m_state = rip_parallel::exclusive_state;
        }
        // Acquires shared access over the lock. Suspends the calling thread until lock is obtained.
        void lock_shared(void)
        {
            // Recursive calls are not supported.
            if (m_state == rip_parallel::shared_state)
            {
                return;
            }
            // If we are in any state, other than 'no lock' and 'shared' - results are undefined.
            // It's safer to throw exception.
            if (m_state != rip_parallel::defer_state)
            {
                throw std::exception("While trying to acquire shared lock, invalid state of upgradable_lock was found. State was: " + m_state);
            }

            // Obtain the shared lock.
            m_mutex.lock_shared();
            // Update the state accordingly.
            m_state = rip_parallel::shared_state;
        }
        // Atomically acquires exclusive access over the lock, without releasing the shared access. Spins the calling thread until upgrade is obtained.
        void lock_upgrade(void)
        {
            // Recursive calls are not supported.
            if (m_state == upgrade_state)
            {
                return;
            }
            else if (m_state == exclusive_state)
            {
                // If we are in 'exclusive' state already - results are undefined.
                // It's safer to throw exception.
                throw std::exception("While trying to upgrade shared lock, invalid state of upgradable_lock was found. State was: " + m_state);
            }
            else if (m_state == defer_state)
            {
                // If we are in 'no lock' state - we need to obtain 'shared' lock, first.
                m_mutex.lock_shared();
            }
            // Update the state accordingly.
            m_state = rip_parallel::upgrade_state;

            // Since we have acquired READ access at this point, it means that any future thread(s) that wants to acquire WRITE access
            // will be suspended, until we release the READ access. That excludes all writters from the equation. In order to atomically
            // upgrade, all we need to handle now are future readers. By calling upgradable_mutex::disable_read we are changing the
            // upgradable_mutex::m_readevent to signaled state. After this point all future thread(s) that wants READ access will be
            // suspended until the upgradable_mutex::enable_read is invoked and upgradable_mutex::m_readevent is changed back to
            // non-signaled state.
            m_mutex.disable_read();

            // Once we have excluded all future WRITE requests and disabled all future READ requests, there is a possibility that some
            // readers are still active, and have not yet completed their READ operations. That's why we will spin and sleep some short
            // amount of time - 10ms in below case, waiting for all readers to complete their operations.
            while (m_mutex.readers_count())
            {
                Sleep(10);
            }

            // Only once there are no active readers, and we have disabled all future READ/WRITE requests - we can then conclude that
            // only upgraders are now active. Last step is to synchronize upgarders between themselves using separate mutex object.
            m_mutex.upgrader_acquire_exclusive_access();

            // Once upgrader reaches THIS POINT, it means that lock was successfully upgraded.
            // EXCLUSIVE work may be performed now.
        }
    private:
        upgradable_mutex& m_mutex;
        upgradable_lock_state m_state = rip_parallel::defer_state;
    };
};

EDIT: I've made another edit, thanks to the @RbMm notes. There was an issue due to lock and readers count not being made atomically but this is resolved now.

\$\endgroup\$
  • 1
    \$\begingroup\$ your code is wrong. thread_1 call lock_shared and suspended before m_readers++; thread_2 also call lock_shared and then lock_upgrade(). m_mutex.readers_count() will be 0 (thread_1 yet not increment m_readers). as result you got at same time - 1 exclusive and one shared owner. \$\endgroup\$ – RbMm Oct 26 '18 at 20:00
  • \$\begingroup\$ that happens because you modify state of your lock in not atomic way. several objects/variable. if thread partial modify this state and will be suspended in process - will be wrong result \$\endgroup\$ – RbMm Oct 26 '18 at 20:03
  • \$\begingroup\$ @RbMm good catch - I've updated the code. Should be no issues now. \$\endgroup\$ – Miloš Ljumović Oct 29 '18 at 15:12
  • \$\begingroup\$ what sense of what you try do ? even if assume that code now correct (i not check deep, but strong doubt) performance of this will be in tens or hundreds times worse compare simply ReleaseSRWLockShared + AcquireSRWLockExclusive \$\endgroup\$ – RbMm Oct 29 '18 at 16:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.