Allow promises to be constructed for non-default-constructible types (#2568)

Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>

stl/inc/future

@@ -184,23 +184,36 @@ struct _State_deleter : _Deleter_base<_Ty> { // manage allocator and deletion st
     _Alloc _My_alloc;
 };
 
+template <class _Ty>
+union _Result_holder {
+    _Result_holder() {}
+    ~_Result_holder() {}
+
+    _Ty _Held_value;
+};
+
 template <class _Ty>
 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 <class _Ty2>
+    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<void*>(_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<mutex>* _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<mutex>* _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<mutex> _Lock(_Mtx);
-        _Set_value_raw(&_Lock, _At_thread_exit);
-    }
-
-    void _Set_value_raw(
-        unique_lock<mutex>* _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<mutex>* _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<T>::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<mutex> _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<is_default_constructible_v<_Ty>, _Ty, _Result_holder<_Ty>> _Result;
     exception_ptr _Exception;
     mutex _Mtx;
     condition_variable _Cond;
@@ -402,7 +424,10 @@ private:
 
     virtual void _Do_notify(unique_lock<mutex>* _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

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

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

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 <atomic>
+#include <cassert>
+#include <exception>
+#include <future>
+#include <system_error>
+#include <thread>
+
+static std::atomic<int> has_default_objects{0};
+static std::atomic<int> no_default_objects{0};
+static std::atomic<int> 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 <class F>
+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 <class T>
+void run_tests() {
+    using Promise = std::promise<T>;
+    using Future  = std::future<T>;
+
+    {
+        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<int> 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<T()> pt([] { return T(7); });
+        Future f = pt.get_future();
+        pt();
+
+        assert(f.get().x == 7);
+    }
+}
+
+int main() {
+    run_tests<has_default>();
+    run_tests<no_default>();
+    run_tests<no_default_or_assign>();
+    assert(has_default_objects == 0);
+    assert(no_default_objects == 0);
+    assert(no_default_or_assign_objects == 0);
+}

tests/tr1/tests/future/test.cpp

@@ -268,13 +268,13 @@ static void call_promise_setter(STD promise<Ty>* 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;
     }
 }