diff mbox

libstdc++/60966 fix synchronisation in std::promise

Message ID 20140517130751.GE29145@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely May 17, 2014, 1:07 p.m. UTC 
This patch implements the correct synchronisation guarantees for
promise::set_value() and promise:;set_exception, so that successful
calls to those functions synchronize with functions that detect the
ready state.

Previously a waiting function could detect a ready state as soon as
the result was stored into the state, but before signalling the
condition variable or returning from std::call_once(). That meant a
waiting function might destroy the std::promise thinking it was
finished with. If the promise released the last reference to the
shared state it would invalidate the pthread_once_t and pthread_cond_t
objects which were still in use.

With this change the lock is held until after the once flag and
condvar have been used, so readiness of the state cannot be detected
until _M_set_result returns. (Note that we still don't take the lock
until after running the setter, since that might be running arbitrary
user-defined code in copy constructors or callable tasks which could
deadlock or result in undefined behaviour if called with the lock held
... not all implementations get this right!)

The second patch is for the release branches, where instead of
changing the function signatures I just take an extra reference to the
shared state in the set_value() and set_exception() members, so that
if the promise is destroyed as soon as the state is made ready it
doesn't release the last refererence and the shared state isn't
destroyed too soon.

Tested x86_64-linux, committed to trunk and 4.9, with a commit to
follow for 4.8 after the branch re-opens.
diff mbox

Patch

commit 6d85f3c11e7f31089e9efbb5bdbee225667de2fe
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri May 16 16:57:17 2014 +0100

    	PR libstdc++/60966
    	* include/std/future (__future_base::_State_baseV2::_M_set_result):
    	Signal condition variable after call_once returns.
    	(__future_base::_State_baseV2::_M_do_set): Do not signal here.
    	(promise::set_value, promise::set_exception): Increment the reference
    	count on the shared state until the function returns.
    	* testsuite/30_threads/promise/60966.cc: New.

diff --git a/libstdc++-v3/include/std/future b/libstdc++-v3/include/std/future
index 717ce71..998e90a 100644
--- a/libstdc++-v3/include/std/future
+++ b/libstdc++-v3/include/std/future
@@ -365,12 +365,14 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       _M_set_result(function<_Ptr_type()> __res, bool __ignore_failure = false)
       {
-        bool __set = __ignore_failure;
+        bool __set = false;
         // all calls to this function are serialized,
         // side-effects of invoking __res only happen once
         call_once(_M_once, &_State_baseV2::_M_do_set, this, ref(__res),
             ref(__set));
-        if (!__set)
+	if (__set)
+	  _M_cond.notify_all();
+	else if (!__ignore_failure)
           __throw_future_error(int(future_errc::promise_already_satisfied));
       }
 
@@ -485,7 +487,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
           lock_guard<mutex> __lock(_M_mutex);
           _M_result.swap(__res);
         }
-        _M_cond.notify_all();
         __set = true;
       }
 
@@ -495,6 +496,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       virtual void _M_complete_async() { }
 
       // Return true if state contains a deferred function.
+      // Caller must own _M_mutex.
       virtual bool _M_has_deferred() const { return false; }
     };
 
@@ -1007,22 +1009,25 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       set_value(const _Res& __r)
       {
+	auto __future = _M_future;
         auto __setter = _State::__setter(this, __r);
-        _M_future->_M_set_result(std::move(__setter));
+        __future->_M_set_result(std::move(__setter));
       }
 
       void
       set_value(_Res&& __r)
       {
+	auto __future = _M_future;
         auto __setter = _State::__setter(this, std::move(__r));
-        _M_future->_M_set_result(std::move(__setter));
+        __future->_M_set_result(std::move(__setter));
       }
 
       void
       set_exception(exception_ptr __p)
       {
+	auto __future = _M_future;
         auto __setter = _State::__setter(__p, this);
-        _M_future->_M_set_result(std::move(__setter));
+        __future->_M_set_result(std::move(__setter));
       }
     };
 
@@ -1105,15 +1110,17 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       set_value(_Res& __r)
       {
+	auto __future = _M_future;
         auto __setter = _State::__setter(this, __r);
-        _M_future->_M_set_result(std::move(__setter));
+        __future->_M_set_result(std::move(__setter));
       }
 
       void
       set_exception(exception_ptr __p)
       {
+	auto __future = _M_future;
         auto __setter = _State::__setter(__p, this);
-        _M_future->_M_set_result(std::move(__setter));
+        __future->_M_set_result(std::move(__setter));
       }
     };
 
@@ -1190,8 +1197,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       set_exception(exception_ptr __p)
       {
+	auto __future = _M_future;
         auto __setter = _State::__setter(__p, this);
-        _M_future->_M_set_result(std::move(__setter));
+        __future->_M_set_result(std::move(__setter));
       }
     };
 
@@ -1217,8 +1225,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   inline void
   promise<void>::set_value()
   {
+    auto __future = _M_future;
     auto __setter = _State::__setter(this);
-    _M_future->_M_set_result(std::move(__setter));
+    __future->_M_set_result(std::move(__setter));
   }
 
 
diff --git a/libstdc++-v3/testsuite/30_threads/promise/60966.cc b/libstdc++-v3/testsuite/30_threads/promise/60966.cc
new file mode 100644
index 0000000..269268b
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/promise/60966.cc
@@ -0,0 +1,67 @@ 
+// { dg-do run { target *-*-freebsd* *-*-netbsd* *-*-linux* *-*-gnu* *-*-solaris* *-*-cygwin *-*-darwin* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++11 -pthread" { target *-*-freebsd* *-*-netbsd* *-*-linux* *-*-gnu* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++11 -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++11 " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+// { dg-require-atomic-builtins "" }
+
+// Copyright (C) 2014 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/>.
+
+// libstdc++/60966
+// This test hangs if std::promise::~promise() destroys the
+// shared state before std::promise::set_value() finishes using it.
+
+#include <future>
+#include <thread>
+#include <vector>
+
+const int THREADS = 10;
+
+void run_task(std::promise<void>* pr)
+{
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  pr->set_value();
+}
+
+int main()
+{
+  std::vector<std::promise<void>*> tasks(THREADS);
+  std::vector<std::thread> threads(THREADS);
+  std::vector<std::future<void>> futures(THREADS);
+
+  for (int i = 0; i < THREADS; ++i)
+  {
+    std::promise<void>* task = new std::promise<void>;
+    tasks[i] = task;
+    futures[i] = task->get_future();
+    threads[i] = std::thread(run_task, task);
+  }
+
+  for (int i = 0; i < THREADS; ++i)
+  {
+    // the temporary future releases the state as soon as wait() returns
+    std::future<void>(std::move(futures[i])).wait();
+    // state is ready, should now be safe to delete promise, so it
+    // releases the shared state too
+    delete tasks[i];
+  }
+
+  for (auto& t : threads)
+    t.join();
+}