From 38ddb14c63a709db6c72a893879f6d2c96a3ec62 Mon Sep 17 00:00:00 2001 From: Mital Ashok Date: Fri, 4 Mar 2022 03:59:21 +0000 Subject: [PATCH] Allow promises to be constructed for non-default-constructible types (#2568) Co-authored-by: Stephan T. Lavavej --- stl/inc/future | 83 +++++--- tests/std/test.lst | 1 + .../env.lst | 4 + .../test.cpp | 184 ++++++++++++++++++ tests/tr1/tests/future/test.cpp | 4 +- 5 files changed, 245 insertions(+), 31 deletions(-) create mode 100644 tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst create mode 100644 tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp diff --git a/stl/inc/future b/stl/inc/future index 1eed67529..66e2eb0f4 100644 --- a/stl/inc/future +++ b/stl/inc/future @@ -184,23 +184,36 @@ struct _State_deleter : _Deleter_base<_Ty> { // manage allocator and deletion st _Alloc _My_alloc; }; +template +union _Result_holder { + _Result_holder() {} + ~_Result_holder() {} + + _Ty _Held_value; +}; + template class _Associated_state { // class for managing associated synchronous state public: using _State_type = _Ty; using _Mydel = _Deleter_base<_Ty>; + // TRANSITION, incorrectly default constructs _Result when _Ty is default constructible _Associated_state(_Mydel* _Dp = nullptr) : _Refs(1), // non-atomic initialization _Exception(), _Retrieved(false), _Ready(false), _Ready_at_thread_exit(false), _Has_stored_result(false), - _Running(false), _Deleter(_Dp) { - // TRANSITION: _Associated_state ctor assumes _Ty is default constructible - } + _Running(false), _Deleter(_Dp) {} virtual ~_Associated_state() noexcept { - if (_Has_stored_result && !_Ready) { // registered for release at thread exit + if (_Already_has_stored_result() && !_Ready) { // registered for release at thread exit _Cond._Unregister(_Mtx); } + + if constexpr (!is_default_constructible_v<_Ty>) { + if (_Has_stored_result) { + _Result._Held_value.~_Ty(); + } + } } void _Retain() { // increment reference count @@ -272,6 +285,8 @@ public: _Rethrow_future_exception(_Exception); } + // TRANSITION: `_Retrieved` should be assigned before `_Exception` is thrown so that a `future::get` + // that throws a stored exception invalidates the future (N4901 [futures.unique.future]/17) _Retrieved = true; _Maybe_run_deferred_function(_Lock); while (!_Ready) { @@ -282,7 +297,22 @@ public: _Rethrow_future_exception(_Exception); } - return _Result; + if constexpr (is_default_constructible_v<_Ty>) { + return _Result; + } else { + return _Result._Held_value; + } + } + + template + void _Emplace_result(_Ty2&& _Val) { + // TRANSITION, incorrectly assigns _Result when _Ty is default constructible + if constexpr (is_default_constructible_v<_Ty>) { + _Result = _STD forward<_Ty2>(_Val); + } else { + ::new (static_cast(_STD addressof(_Result._Held_value))) _Ty(_STD forward<_Ty2>(_Val)); + _Has_stored_result = true; + } } void _Set_value(const _Ty& _Val, bool _At_thread_exit) { // store a result @@ -292,11 +322,11 @@ public: void _Set_value_raw(const _Ty& _Val, unique_lock* _Lock, bool _At_thread_exit) { // store a result while inside a locked block - if (_Has_stored_result) { + if (_Already_has_stored_result()) { _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); } - _Result = _Val; + _Emplace_result(_Val); _Do_notify(_Lock, _At_thread_exit); } @@ -307,25 +337,11 @@ public: void _Set_value_raw(_Ty&& _Val, unique_lock* _Lock, bool _At_thread_exit) { // store a result while inside a locked block - if (_Has_stored_result) { - _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); - } - - _Result = _STD forward<_Ty>(_Val); - _Do_notify(_Lock, _At_thread_exit); - } - - void _Set_value(bool _At_thread_exit) { // store a (void) result - unique_lock _Lock(_Mtx); - _Set_value_raw(&_Lock, _At_thread_exit); - } - - void _Set_value_raw( - unique_lock* _Lock, bool _At_thread_exit) { // store a (void) result while inside a locked block - if (_Has_stored_result) { + if (_Already_has_stored_result()) { _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); } + _Emplace_result(_STD forward<_Ty>(_Val)); _Do_notify(_Lock, _At_thread_exit); } @@ -336,10 +352,12 @@ public: void _Set_exception_raw(exception_ptr _Exc, unique_lock* _Lock, bool _At_thread_exit) { // store a result while inside a locked block - if (_Has_stored_result) { + if (_Already_has_stored_result()) { _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); } + _STL_ASSERT(_Exc != nullptr, "promise::set_exception/set_exception_at_thread_exit called with a null " + "std::exception_ptr, which is invalid per N4901 32.9.6 [futures.promise]/20"); _Exception = _Exc; _Do_notify(_Lock, _At_thread_exit); } @@ -352,8 +370,12 @@ public: return _Ready_at_thread_exit; } - bool _Already_has_stored_result() const { - return _Has_stored_result; + bool _Already_has_stored_result() const { // Has a result or an exception + if constexpr (is_default_constructible_v<_Ty>) { + return _Has_stored_result; + } else { + return _Has_stored_result || _Exception; + } } bool _Already_retrieved() const { @@ -362,7 +384,7 @@ public: void _Abandon() { // abandon shared state unique_lock _Lock(_Mtx); - if (!_Has_stored_result) { // queue exception + if (!_Already_has_stored_result()) { // queue exception future_error _Fut(make_error_code(future_errc::broken_promise)); _Set_exception_raw(_STD make_exception_ptr(_Fut), &_Lock, false); } @@ -383,7 +405,7 @@ protected: } public: - _Ty _Result; + conditional_t, _Ty, _Result_holder<_Ty>> _Result; exception_ptr _Exception; mutex _Mtx; condition_variable _Cond; @@ -402,7 +424,10 @@ private: virtual void _Do_notify(unique_lock* _Lock, bool _At_thread_exit) { // notify waiting threads // TRANSITION, ABI: This is virtual, but never overridden. - _Has_stored_result = true; + if constexpr (is_default_constructible_v<_Ty>) { + _Has_stored_result = true; + } + if (_At_thread_exit) { // notify at thread exit _Cond._Register(*_Lock, &_Ready); } else { // notify immediately diff --git a/tests/std/test.lst b/tests/std/test.lst index af4d747a4..8379ab038 100644 --- a/tests/std/test.lst +++ b/tests/std/test.lst @@ -191,6 +191,7 @@ tests\GH_002030_asan_annotate_vector tests\GH_002039_byte_is_not_trivially_swappable tests\GH_002058_debug_iterator_race tests\GH_002120_streambuf_seekpos_and_seekoff +tests\GH_002488_promise_not_default_constructible_types tests\LWG2597_complex_branch_cut tests\LWG3018_shared_ptr_function tests\LWG3146_excessive_unwrapping_ref_cref diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst b/tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst new file mode 100644 index 000000000..e970fe46a --- /dev/null +++ b/tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst @@ -0,0 +1,4 @@ +# Copyright (c) Microsoft Corporation. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +RUNALL_INCLUDE ..\native_matrix.lst diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp new file mode 100644 index 000000000..0f4dedb10 --- /dev/null +++ b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp @@ -0,0 +1,184 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include +#include +#include +#include +#include +#include + +static std::atomic has_default_objects{0}; +static std::atomic no_default_objects{0}; +static std::atomic no_default_or_assign_objects{0}; + +struct has_default { + has_default() : x(0xbad) { + ++has_default_objects; + } + explicit has_default(int n) : x(n) { + ++has_default_objects; + } + has_default(const has_default& v) : x(v.x) { + ++has_default_objects; + } + + ~has_default() { + --has_default_objects; + } + + int x; +}; + +struct no_default { + no_default() = delete; + explicit no_default(int n) : x(n) { + ++no_default_objects; + } + no_default(const no_default& v) : x(v.x) { + ++no_default_objects; + } + + ~no_default() { + --no_default_objects; + } + + int x; +}; + +struct no_default_or_assign { + no_default_or_assign() = delete; + explicit no_default_or_assign(int n) : x(n) { + ++no_default_or_assign_objects; + } + no_default_or_assign(const no_default_or_assign& v) : x(v.x) { + ++no_default_or_assign_objects; + } + + void operator=(const no_default_or_assign&) = delete; + + ~no_default_or_assign() { + --no_default_or_assign_objects; + } + + int x; +}; + +template +void assert_throws_future_error(F f, std::error_code expected_code) { + try { + f(); + } catch (const std::future_error& e) { + assert(e.code() == expected_code); + return; + } catch (...) { + } + assert(false); +} + +template +void run_tests() { + using Promise = std::promise; + using Future = std::future; + + { + Promise p; + p.set_value(T(4)); + assert(p.get_future().get().x == 4); + } + + { + Promise p; + Future f = p.get_future(); + T v(10); + p.set_value(v); + assert(f.get().x == 10); + assert_throws_future_error([&] { p.set_value(v); }, std::future_errc::promise_already_satisfied); + assert_throws_future_error([&] { f.get(); }, std::future_errc::no_state); + assert_throws_future_error([&] { p.get_future().get(); }, std::future_errc::future_already_retrieved); + } + + { + Promise p; + Future f = p.get_future(); + p.set_exception(std::make_exception_ptr(5)); + try { + f.get(); + assert(false); + } catch (int i) { + assert(i == 5); + } catch (...) { + assert(false); + } + } + + { + Promise p; + Future f = p.get_future(); + p.set_exception(std::make_exception_ptr(3)); + assert_throws_future_error([&] { p.set_value(T(2)); }, std::future_errc::promise_already_satisfied); + try { + f.get(); + assert(false); + } catch (int i) { + assert(i == 3); + } catch (...) { + assert(false); + } + } + + { + Promise p; + Future f = p.get_future(); + std::atomic failures{0}; + int succeeded = -1; + auto make_thread = [&](int n) { + return std::thread([&, n] { + try { + p.set_value(T(n)); + } catch (std::future_error) { + ++failures; + return; + } + succeeded = n; + }); + }; + std::thread threads[]{make_thread(0), make_thread(1), make_thread(2), make_thread(3), make_thread(4), + make_thread(5), make_thread(6), make_thread(7)}; + + for (auto& t : threads) { + t.join(); + } + + assert(failures == 7); + assert(succeeded != -1 && f.get().x == succeeded); + } + + { + (void) std::async(std::launch::async, [] { return T(16); }); + (void) std::async(std::launch::async, [] { + const T x(40); + return x; + }); + + Future f = std::async(std::launch::async, [] { return T(23); }); + assert(f.get().x == 23); + } + + { + std::packaged_task pt([] { return T(7); }); + Future f = pt.get_future(); + pt(); + + assert(f.get().x == 7); + } +} + +int main() { + run_tests(); + run_tests(); + run_tests(); + assert(has_default_objects == 0); + assert(no_default_objects == 0); + assert(no_default_or_assign_objects == 0); +} diff --git a/tests/tr1/tests/future/test.cpp b/tests/tr1/tests/future/test.cpp index 08a0f3fb5..1c8ca7b72 100644 --- a/tests/tr1/tests/future/test.cpp +++ b/tests/tr1/tests/future/test.cpp @@ -268,13 +268,13 @@ static void call_promise_setter(STD promise* pr, int which) { // try to set pr->set_value(3); break; case 1: - pr->set_exception(STD exception_ptr()); + pr->set_exception(STD make_exception_ptr(0)); break; case 2: pr->set_value_at_thread_exit(3); break; case 3: - pr->set_exception_at_thread_exit(STD exception_ptr()); + pr->set_exception_at_thread_exit(STD make_exception_ptr(0)); break; } }