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.
commit e0b7b952168ac1106771da549cf11878d0391f57
Author: redi <redi@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Sat May 17 12:58:46 2014 +0000
PR libstdc++/60966
* include/std/future (__future_base::_State_baseV2::_M_set_result):
Pass lock into _M_do_set and hold it until the function returns.
Signal condition variable after call_once completes.
(__future_base::_State_baseV2::_M_do_set): Use lock argument. Do not
signal here.
* testsuite/30_threads/promise/60966.cc: New.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@210556 138bc75d-0d04-0410-961f-82ee72b054a4
diff --git a/libstdc++-v3/include/std/future b/libstdc++-v3/include/std/future
index 717ce71..ea11f3f 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;
+ unique_lock<mutex> __lock(_M_mutex, defer_lock);
// 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)
+ call_once(_M_once, &_State_baseV2::_M_do_set, this,
+ ref(__res), ref(__lock));
+ if (__lock.owns_lock())
+ _M_cond.notify_all();
+ else if (!__ignore_failure)
__throw_future_error(int(future_errc::promise_already_satisfied));
}
@@ -478,15 +480,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
private:
void
- _M_do_set(function<_Ptr_type()>& __f, bool& __set)
+ _M_do_set(function<_Ptr_type()>& __f, unique_lock<mutex>& __lock)
{
- _Ptr_type __res = __f();
- {
- lock_guard<mutex> __lock(_M_mutex);
- _M_result.swap(__res);
- }
- _M_cond.notify_all();
- __set = true;
+ _Ptr_type __res = __f(); // do not hold lock while running setter
+ __lock.lock();
+ _M_result.swap(__res);
}
bool _M_ready() const noexcept { return static_cast<bool>(_M_result); }
@@ -495,6 +493,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; }
};
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();
+}
commit 6d85f3c11e7f31089e9efbb5bdbee225667de2fe
Author: Jonathan Wakely <jwak...@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();
+}
