Upgrade lock is not acquired when previous upgrade lock releases if another read lock is present

[fred@…]

Thread 1 acquires a read lock on a shared mutex
Thread 2 acquires an upgrade lock on the shared mutex
Thread 3 tries to acquire an upgrade lock on the shared mutex, waits for it
Thread 2 releases the lock

At this point, I would expect thread 3 to acquire the lock, but it does not do it until thread 1 releases its read lock.

This seems to be caused by the "shared_mutex ::unlock_upgrade()" method in thread/pthread/shared_mutex.hpp, that only calls "release_waiters()" if it is the last reader, ignoring that another upgrade lock might be waiting to take the lock.

I suppose it could be fixed by maintaining a count of the number of threads waiting for an upgrade lock, and to signal the shared condition if this is non-zero.

Here is an example program:

#include <iostream>
#include <boost/thread.hpp>

class Test
{
public:
  void m1()
  {
    std::cout << "m1 - Trying to take an shared lock" << std::endl;
    boost::shared_lock<boost::shared_mutex> lock(_mutex);
    std::cout << "m1 - Took a shared lock" << std::endl;
    sleep(5);
    std::cout << "m1 - Released the lock" << std::endl;
  }

  void m2()
  {
    sleep(1);
    std::cout << "m2 - Trying to take an upgradable lock" << std::endl;
    boost::upgrade_lock<boost::shared_mutex> lock(_mutex);
    std::cout << "m2 - Took an upgradable lock" << std::endl;
    sleep(1);
    std::cout << "m2 - Released an upgradable lock" << std::endl;
  }
  
  void m3()
  {
    sleep(2);
    std::cout << "m3 - Trying to take an upgradable lock" << std::endl;
    boost::upgrade_lock<boost::shared_mutex> lock(_mutex);
    std::cout << "m3 - Took an upgradable lock" << std::endl;
    
    std::cout << "m3 - Releasing locks" << std::endl;
  }

  void run()
  {
    boost::thread t1(&Test::m1, this);  
    boost::thread t2(&Test::m2, this);  
    m3();
    t1.join();
    t2.join();
  }
  
private:
  boost::shared_mutex _mutex;
};

int main()
{
  Test test;
  test.run();

  return 0;
}

Results:

m1 - Trying to take an shared lock
m1 - Took a shared lock
m2 - Trying to take an upgradable lock
m2 - Took an upgradable lock
m3 - Trying to take an upgradable lock
m2 - Released an upgradable lock
m1 - Released the lock
m3 - Took an upgradable lock
m3 - Releasing locks

Expected:

m1 - Trying to take an shared lock
m1 - Took a shared lock
m2 - Trying to take an upgradable lock
m2 - Took an upgradable lock
m3 - Trying to take an upgradable lock
m2 - Released an upgradable lock
m3 - Took an upgradable lock
m3 - Releasing locks
m1 - Released the lock

Tested in Boost 1.42, visually checked that Trunk would have the same behaviour.

[viboes]

It seems to me that this is by definition of the provided feature.

From the documentation: "This is an extension to the multiple-reader / single-write model provided by the SharedLockable concept: a single thread may have upgradable ownership at the same time as others have shared ownership. The thread with upgradable ownership may at any time attempt to upgrade that ownership to exclusive ownership. If no other threads have shared ownership, the upgrade is completed immediately, and the thread now has exclusive ownership, which must be relinquished by a call to unlock(), just as if it had been acquired by a call to lock().

If a thread with upgradable ownership tries to upgrade whilst other threads have shared ownership, the attempt will fail and the thread will block until exclusive ownership can be acquired. " Of course the implementation could give priority to writers, but this should be implemented in another class.

What is then wrong?

Moved to Support request until resolution clarified.

[fred@…]

Quoted: "the thread will block until exclusive ownership can be acquired".

In the example I gave, m3 could have acquired ownership just after m2 released its upgradable lock, because only a read lock was taken at that point, by m1. And yet, it did wait until after m1 released the lock to acquire it.

I understand this sentence of the documentation as meaning the acquisition is eager, but it seems that it is not. Is that correct?

[fred@…]

The quote in my previous comment is not relevant, actually, sorry about this. My issue is specifically when upgradable ownership may be acquired, because only readers are left, but it is not.

[viboes]

Coming back to you example

m1 - Trying to take an shared lock m1 - Took a shared lock m2 - Trying to take an upgradable lock m2 - Took an upgradable lock m3 - Trying to take an upgradable lock m2 - Released an upgradable lock m3 - Took an upgradable lock m3 - Releasing locks m1 - Released the lock

Whether these sequence appear before or after in the trace are irrelevant.

m3 - Took an upgradable lock m3 - Releasing locks

m1 - Released the lock

It is up to the scheduler to execute m1 or m3, as both are active threads, no one is blocking for the other. Note that m1 had already a shared lock, so it can continue to be executed independently of whether m3 takes the upgrade lock.

The following sequence could also be possible

m3 - Took an upgradable lock m1 - Released the lock m3 - Releasing locks

[fred@…]

Viboes, agreed, last steps don't matter. My concern here is on the 7th step:

m1 - Trying to take an shared lock
m1 - Took a shared lock
m2 - Trying to take an upgradable lock
m2 - Took an upgradable lock
m3 - Trying to take an upgradable lock
m2 - Released an upgradable lock
here I would expect m3 to be able to take the upgradable lock, even though m1 still has a read lock
m1 - Released the lock

Because the "release_waiters()" is only called when there are no more readers, we ignore the case when a thread wanting an upgradable lock should be waken up when there are still read locks going on.

This makes the locking non-eager.

[viboes]

Replying to fred@…:

Viboes, agreed, last steps don't matter. My concern here is on the 7th step:

m1 - Trying to take an shared lock
m1 - Took a shared lock
m2 - Trying to take an upgradable lock
m2 - Took an upgradable lock
m3 - Trying to take an upgradable lock
m2 - Released an upgradable lock
here I would expect m3 to be able to take the upgradable lock, even though m1 still has a read lock

m1 - Released the lock

Because the "release_waiters()" is only called when there are no more readers, we ignore the case when a thread wanting an upgradable lock should be waken up when there are still read locks going on.

This makes the locking non-eager.

You are right. The fact that in

        void unlock_upgrade()
        {
            boost::mutex::scoped_lock lk(state_change);
            state.upgrade=false;
            bool const last_reader=!--state.shared_count;

            if(last_reader)
            {
                state.exclusive_waiting_blocked=false;
                release_waiters();
            }
        }

state.upgrade has been set at least

shared_cond.notify_all();

must be called.

Please could you try the following

        void unlock_upgrade()
        {
            boost::mutex::scoped_lock lk(state_change);
            state.upgrade=false;
            bool const last_reader=!--state.shared_count;

            if(last_reader)
            {
                state.exclusive_waiting_blocked=false;
                release_waiters();
            } else {
              shared_cond.notify_all();
            }
        }

[viboes]

Committed in trunk at revision [76543].

[viboes]

Committed in release branch at [78543]