Skip to content

Commit

Permalink
rollback and use RAII.
Browse files Browse the repository at this point in the history
  • Loading branch information
@viboes
viboes committed May 6, 2017
1 parent 24188f2 commit ace2b8f
Showing 1 changed file with 76 additions and 82 deletions.
158 changes: 76 additions & 82 deletions include/boost/thread/win32/condition_variable.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,37 +138,37 @@ namespace boost
detail::win32::ReleaseSemaphore(wake_sem,count_to_wake,0);
}

// template<typename lock_type>
// struct relocker
// {
// BOOST_THREAD_NO_COPYABLE(relocker)
// lock_type& _lock;
// bool _unlocked;
//
// relocker(lock_type& lock_):
// _lock(lock_), _unlocked(false)
// {}
// void unlock()
// {
// if ( ! _unlocked )
// {
// _lock.unlock();
// _unlocked=true;
// }
// }
// void lock()
// {
// if ( _unlocked )
// {
// _lock.lock();
// _unlocked=false;
// }
// }
// ~relocker() BOOST_NOEXCEPT_IF(false)
// {
// lock();
// }
// };
template<typename lock_type>
struct relocker
{
BOOST_THREAD_NO_COPYABLE(relocker)
lock_type& _lock;
bool _unlocked;

relocker(lock_type& lock_):
_lock(lock_), _unlocked(false)
{}
void unlock()
{
if ( ! _unlocked )
{
_lock.unlock();
_unlocked=true;
}
}
void lock()
{
if ( _unlocked )
{
_lock.lock();
_unlocked=false;
}
}
~relocker() BOOST_NOEXCEPT_IF(false)
{
lock();
}
};


entry_ptr get_wait_entry()
Expand All @@ -194,65 +194,59 @@ namespace boost
}
}

// struct entry_manager
// {
// entry_ptr const entry;
// boost::mutex& internal_mutex;
//
// BOOST_THREAD_NO_COPYABLE(entry_manager)
// entry_manager(entry_ptr const& entry_, boost::mutex& mutex_):
// entry(entry_), internal_mutex(mutex_)
// {}
//
// ~entry_manager() BOOST_NOEXCEPT_IF(false)
// {
// boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
// entry->remove_waiter();
// }
//
// list_entry* operator->()
// {
// return entry.get();
// }
// };
struct entry_manager
{
entry_ptr const entry;
boost::mutex& internal_mutex;


protected:
template<typename lock_type>
bool do_wait(lock_type& lock,timeout abs_time)
{
//relocker<lock_type> locker(lock);
//entry_manager entry(get_wait_entry(), internal_mutex);
entry_ptr const entry = get_wait_entry();
try {
lock.unlock();

bool woken=false;
while(!woken)
BOOST_THREAD_NO_COPYABLE(entry_manager)
entry_manager(entry_ptr const& entry_, boost::mutex& mutex_):
entry(entry_), internal_mutex(mutex_)
{}

void remove_waiter()
{
if(!entry->wait(abs_time))
{
{
boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
entry->remove_waiter();
}
lock.lock();
return false;
}
woken=entry->woken();
if (entry) {
boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
entry->remove_waiter();
entry = 0;
}
}
~entry_manager() BOOST_NOEXCEPT_IF(false)
{
boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
entry->remove_waiter();
remove_waiter();
}
lock.lock();
return woken;
}
catch (...)

list_entry* operator->()
{
return entry.get();
}
};


protected:
template<typename lock_type>
bool do_wait(lock_type& lock,timeout abs_time)
{
relocker<lock_type> locker(lock);
entry_manager entry(get_wait_entry(), internal_mutex);
locker.unlock();

bool woken=false;
while(!woken)
{
lock.lock();
throw;
if(!entry->wait(abs_time))
{
return false;
}

woken=entry->woken();
}
// do it here to avoid throwing on the destructor
entry->remove_waiter();
locker.lock();
return woken;
}

template<typename lock_type,typename predicate_type>
Expand Down

3 comments on commit ace2b8f

@viboes
Copy link
Collaborator Author

@viboes viboes commented on ace2b8f Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I suspect that this shouldn't compile then. It is weir that no body has discover this bug yet. I hav eno access to Windows. Please could you provide a PR.

@panyusko
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I think this has very little probability to happen, so no one found this. But we use the condition variables very actively in our project (many and many of wait/notify calls per second). And this works fine in Boost 1.64 and stops work in 1.65. I already tried a fix (replaced operator -> with dot) in our environment and it compiles and works as expected.
entry is a local variable in bool do_wait(lock_type& lock,timeout abs_time) function. Early (in 1.64) its destructor called

boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
entry->remove_waiter();

And now (in 1.65) you first call
entry->remove_waiter();
what is equivalent to
entry.entry->remove_waiter();
and finally the destructor of entry additionally calls entry_manager::remove_waiter function

if (entry) {
     boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
     entry->remove_waiter();
     entry = 0;
}

so remove_waiter function of entry_manager::entry member will be called twice and the first call will be without locking of internal_mutex.

@viboes
Copy link
Collaborator Author

@viboes viboes commented on ace2b8f Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Using the same name for the function was an source of errors.

Thanks for catching this.

Please sign in to comment.