Boost C++ Libraries: Ticket #7461: detail::win32::ReleaseSemaphore may be called with count_to_release equal to 0

summary changed

Tue, 02 Oct 2012 20:01:34 GMT

summary detail::win32::ReleaseSemaphore may be called with count_to_release euql to 0 → detail::win32::ReleaseSemaphore may be called with count_to_release equal to 0

owner, status, version changed

viboes — Tue, 09 Oct 2012 23:35:45 GMT

owner changed from Anthony Williams to viboes
status new → assigned
version Boost 1.52.0 → Boost 1.51.0

viboes — Wed, 10 Oct 2012 23:21:23 GMT

Hi,

I don't know how the tool you are using has deduced such an issue.

The count_to_release parameter seems to be always > 0.

Please let me know if I'm missing something.

Either it is called with 1

                    for(generation_list::iterator it=generations.begin(),
                            end=generations.end();
                        it!=end;++it)
                    {
                        (*it)->release(1);
                    }

or with the contents of waiters.

            void release_waiters()
            {
                release(detail::interlocked_read_acquire(&waiters));
            }

This variable is initialized as follows

                waiters(1),notified(false),references(0)

and updated by

            void add_waiter()
            {
                BOOST_INTERLOCKED_INCREMENT(&waiters);
            }
            void remove_waiter()
            {
                BOOST_INTERLOCKED_DECREMENT(&waiters);
            }

But remove_waiter is always called after add_waiter

            entry_ptr get_wait_entry()
            {
                boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
                if(!wake_sem)
                {
                    wake_sem=detail::win32::create_anonymous_semaphore(0,LONG_MAX);
                    BOOST_ASSERT(wake_sem);
                }
                detail::interlocked_write_release(&total_count,total_count+1);
                if(generations.empty() || generations.back()->is_notified())
                {
                    entry_ptr new_entry(new list_entry(wake_sem));
                    generations.push_back(new_entry);
                    return new_entry;
                }
                else
                {
                    generations.back()->add_waiter();
                    return generations.back();
                }
            }
            struct entry_manager
            {
                entry_ptr const entry;
                BOOST_THREAD_NO_COPYABLE(entry_manager)
                entry_manager(entry_ptr const& entry_):
                    entry(entry_)
                {}
                ~entry_manager()
                {
                    entry->remove_waiter();
                }
                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());
                locker.unlock();
                bool woken=false;
                while(!woken)
                {
                    if(!entry->wait(abs_time))
                    {
                        return false;
                    }
                    woken=entry->woken();
                }
                return woken;
            }

Any deep explanation of on which circumstances this issue appear is welcome.

jsbache@… — Thu, 11 Oct 2012 17:25:48 GMT

I apologize for the terseness. Here are a few detais. When I notice a zero count, the stack is:

c4test.exe!boost::detail::basic_cv_list_entry::release(unsigned int count_to_release=0) Line 68 c4test.exe!boost::detail::basic_cv_list_entry::release_waiters() Line 73

c4test.exe!boost::detail::basic_condition_variable::notify_all() Line 285 + 0x1a bytes

My conditional breakpoint (condition: "count_to_release == 0) is on the following line (before we modify notified)

   void release(unsigned count_to_release)
   {
   --break--       notified=true;

When I hit this, code I have the following state of the local variables. Note that notified = true at this point in time,

it	{px=0x0000000001dde7b0 }
	[ptr]	0x0000000001dd75b8 {px=0x0000000001dde7b0 }
	px	0x0000000001dde7b0
	semaphore	{handle_to_manage=0x0000000000000100 }
	wake_sem	{handle_to_manage=0x0000000000000184 }
	waiters		0
	notified	true
	references	1
this	0x00000000003dee28
	internal_mutex	{...}	boost::mutex
	total_count	0	long
	active_generation_count	0	unsigned int
	generations	[2]({px=0x0000000001dde630 },{px=0x0000000001dde7b0 })
	wake_sem	{handle_to_manage=0x0000000000000198 }

viboes — Fri, 12 Oct 2012 22:18:58 GMT

Thanks for the details. Please, could you attach the example? Could you see when waiters become 0?

viboes — Fri, 12 Oct 2012 22:42:24 GMT

Hi again,

I think I have an hint. Could you check if defining ~entry_manager as follows avoid the problem?

                ~entry_manager()
                {
                  if(! entry->is_notified())
                  {
                    entry->remove_waiter();
                  }
                }

jsbache@… — Fri, 12 Oct 2012 23:05:57 GMT

The proposed solution does solve the issue, meaning that with this change I no longer observe a call to basic_cv_list_entry::release with count_to_release equal to 0.

viboes — Sat, 13 Oct 2012 13:46:49 GMT

Replying to jsbache@…:

The proposed solution does solve the issue, meaning that with this change I no longer observe a call to basic_cv_list_entry::release with count_to_release equal to 0.

Thanks for checking. I will apply the modification and run the test on windows as soon as i have access to a windows machine.

milestone changed

viboes — Sat, 20 Oct 2012 09:30:21 GMT

milestone To Be Determined → Boost 1.52.0

status changed; resolution set

viboes — Sat, 20 Oct 2012 15:20:18 GMT

status assigned → closed
resolution → fixed

Committed revision [81024].

status changed; resolution deleted

viboes — Thu, 08 Nov 2012 18:53:50 GMT

status closed → reopened
resolution fixed

Hi,

it seems that the patch introduce a severe restriction, see #7657. I will rollback the change and take the time to analyze your issue.

milestone changed

viboes — Sat, 10 Nov 2012 11:50:10 GMT

milestone Boost 1.52.0 → To Be Determined

status changed

viboes — Sun, 09 Jun 2013 20:13:13 GMT

status reopened → new

status changed

viboes — Sun, 09 Jun 2013 20:13:26 GMT

status new → assigned

viboes — Sat, 14 Sep 2013 15:24:39 GMT

Hi,

I don't reach to understand how your issue can arrive.

Could you add a small example reproduce the issue?

viboes — Sun, 15 Sep 2013 16:12:30 GMT

I suspect that the following sequence is possible.

thread A call wait and it stands in [1] below. The value for waiters is 1.
thread B notify_all and after wake the waiters in [2] the scheduler choose the ready thread A.
thread A ends the call to do_wait and the entry_manager destructor calls. The value for waiters is 0. in [3] the scheduler choose the ready thread B.
thread B calls release_waiters() in [4] which calls to release(0).

Current Code

            template<typename lock_type>
            bool do_wait(lock_type& lock,timeout abs_time)
            {
                relocker<lock_type> locker(lock);
                entry_manager entry(get_wait_entry());
                locker.unlock();
                bool woken=false;
                while(!woken)
                {
                    if(!entry->wait(abs_time)) // [1]
                    {
                        return false;
                    }
                    woken=entry->woken();
                }
                return woken;
            } // [3]
            void notify_all() BOOST_NOEXCEPT
            {
                if(detail::interlocked_read_acquire(&total_count))
                {
                    boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
                    if(!total_count)
                    {
                        return;
                    }
                    wake_waiters(total_count); // [2]
                    for(generation_list::iterator it=generations.begin(),
                            end=generations.end();
                        it!=end;++it)
                    {
                        (*it)->release_waiters(); [4]
                    }
                    generations.clear();
                    wake_sem=detail::win32::handle(0);
                }
            }

I suspect that the call to entry->remove_waiter() inside entry_manager() must be protected

Could some one check if the following patch for boost/thread/win32/condition_variable.hpp works

===================================================================
--- condition_variable.hpp	(revision 85663)
+++ condition_variable.hpp	(working copy)
@@ -191,18 +191,17 @@
             struct entry_manager
             {
                 entry_ptr const entry;
+                boost::mutex& internal_mutex;
                 BOOST_THREAD_NO_COPYABLE(entry_manager)
-                entry_manager(entry_ptr const& entry_):
-                    entry(entry_)
+                entry_manager(entry_ptr const& entry_, boost::mutex& mutex_):
+                    entry(entry_), internal_mutex(mutex_)
                 {}
                 ~entry_manager()
                 {
-                  //if(! entry->is_notified()) // several regression #7657
-                  {
+                    boost::lock_guard<boost::mutex> internal_lock(internal_mutex);
                     entry->remove_waiter();
-                  }
                 }
                 list_entry* operator->()
@@ -218,7 +217,7 @@
             {
                 relocker<lock_type> locker(lock);
-                entry_manager entry(get_wait_entry());
+                entry_manager entry(get_wait_entry(), internal_mutex);
                 locker.unlock();

milestone changed

viboes — Tue, 17 Sep 2013 21:01:37 GMT

milestone To Be Determined → Boost 1.55.0

Committed revision [85733].

status changed; resolution set

viboes — Sat, 21 Sep 2013 20:47:37 GMT

status assigned → closed
resolution → fixed

Committed revision [85815].