Patchwork libstdc++/50862 fix deadlock in condition_variable_any

login
register
mail settings
Submitter Jonathan Wakely
Date Oct. 25, 2011, 8:56 p.m.
Message ID <CAH6eHdSrWLsGGqWvUFXsLFeebxzMYj-9CuJ0a7ZhcSoht4Tczw@mail.gmail.com>
Download mbox | patch
Permalink /patch/121787/
State New
Headers show

Comments

Jonathan Wakely - Oct. 25, 2011, 8:56 p.m.
PR libstdc++/50862
        * include/std/condition_variable (condition_variable_any::wait): Fix
        deadlock and ensure _Lock::lock() is called on exit.
        (condition_variable_any::native_handle): Remove, as per LWG 1500.
        * testsuite/30_threads/condition_variable_any/50862.cc: New.

Tested x86_64-linux, committed to trunk.

I plan to fix this on the 4.6 branch too, it's a shame it's too late for 4.6.2

Patch

Index: include/std/condition_variable
===================================================================
--- include/std/condition_variable	(revision 180411)
+++ include/std/condition_variable	(working copy)
@@ -203,10 +203,19 @@ 
       void
       wait(_Lock& __lock)
       {
-        unique_lock<mutex> __my_lock(_M_mutex);
-        __lock.unlock();
-        _M_cond.wait(__my_lock);
-        __lock.lock();
+	// scoped unlock - unlocks in ctor, re-locks in dtor
+	struct _Unlock {
+	  explicit _Unlock(_Lock& __lk) : _M_lock(__lk) { __lk.unlock(); }
+	  ~_Unlock() { _M_lock.lock(); }
+	  _Lock& _M_lock;
+	};
+
+	unique_lock<mutex> __my_lock(_M_mutex);
+	_Unlock __unlock(__lock);
+	// _M_mutex must be unlocked before re-locking __lock so move
+	// ownership of _M_mutex lock to an object with shorter lifetime.
+	unique_lock<mutex> __my_lock2(std::move(__my_lock));
+	_M_cond.wait(__my_lock2);
       }
       
 
@@ -254,10 +263,6 @@ 
       wait_for(_Lock& __lock,
 	       const chrono::duration<_Rep, _Period>& __rtime, _Predicate __p)
       { return wait_until(__lock, __clock_t::now() + __rtime, std::move(__p)); }
-
-    native_handle_type
-    native_handle()
-    { return _M_cond.native_handle(); }
   };
 
   // @} group condition_variables
Index: testsuite/30_threads/condition_variable_any/50862.cc
===================================================================
--- testsuite/30_threads/condition_variable_any/50862.cc	(revision 0)
+++ testsuite/30_threads/condition_variable_any/50862.cc	(revision 0)
@@ -0,0 +1,75 @@ 
+// { dg-do run { target *-*-freebsd* *-*-netbsd* *-*-linux* *-*-solaris* *-*-cygwin *-*-darwin* alpha*-*-osf* mips-sgi-irix6* } }
+// { dg-options " -std=gnu++0x -pthread" { target *-*-freebsd* *-*-netbsd* *-*-linux* alpha*-*-osf* mips-sgi-irix6* } }
+// { dg-options " -std=gnu++0x -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++0x " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+ 
+// Copyright (C) 2011 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>. 
+
+#include <condition_variable>
+#include <thread>
+#include <mutex>
+#include <array>
+#include <sstream>
+
+struct scoped_thread
+{
+  ~scoped_thread() { if (t.joinable()) t.join(); }
+  std::thread t;
+};
+
+int main()
+{
+  typedef std::unique_lock<std::mutex> Lock;
+
+  std::mutex                  m;
+  std::condition_variable_any cond;
+  unsigned int                product=0;
+  const unsigned int          count=10;
+
+  // writing to stream causes timing changes which makes deadlock easier
+  // to reproduce - do not remove
+  std::ostringstream out;
+
+  // create consumers
+  std::array<scoped_thread, 2> threads;
+  for(size_t i=0; i<threads.size(); ++i)
+    threads[i].t = std::thread( [&] {
+	  for(unsigned int i=0; i<count; ++i)
+	  {
+	    std::this_thread::yield();
+	    Lock lock(m);
+	    while(product==0)
+	      cond.wait(lock);
+	    out << "got product " << std::this_thread::get_id() << ' ' << product << std::endl;
+	    --product;
+	  }
+	} );
+
+  // single producer
+  for(size_t i=0; i<threads.size()*count; ++i)
+  {
+    std::this_thread::yield();
+    Lock lock(m);
+    ++product;
+    out << "setting product " << std::this_thread::get_id() << ' ' << product << std::endl;
+    cond.notify_one();
+  }
+
+}