From facbd553cca8774d9df4fa01ad1d9a832d36eb1e Mon Sep 17 00:00:00 2001 From: Sean Parent Date: Mon, 4 Mar 2024 13:44:29 -0600 Subject: [PATCH] General Cleanup (#539) * Some lint fixes for the chain library CI. * Cleanup - Cleaned up mutex usage in await to eliminate a flag, and directly protect the element being modified. - Fixed a memory leak in Windows if scheduling work fails in the default executor. - Made package_task<> move-only which simplifies promise by removing promise count. - Removed unused reduction_failed error code. - Added a reduction test for future> --- stlab/concurrency/await.hpp | 18 ++++-------- stlab/concurrency/default_executor.hpp | 23 +++++++++------ stlab/concurrency/future.hpp | 39 +++++++++----------------- test/CMakeLists.txt | 3 +- test/future_reduction_tests.cpp | 25 +++++++++++++++++ 5 files changed, 61 insertions(+), 47 deletions(-) create mode 100644 test/future_reduction_tests.cpp diff --git a/stlab/concurrency/await.hpp b/stlab/concurrency/await.hpp index 0e24c470..97bb58f6 100644 --- a/stlab/concurrency/await.hpp +++ b/stlab/concurrency/await.hpp @@ -97,15 +97,12 @@ T await(future x) { std::mutex m; std::condition_variable condition; - bool flag{false}; - future result; auto hold = std::move(x).recover(immediate_executor, [&](future&& r) { - result = std::move(r); { std::unique_lock lock{m}; - flag = true; + result = std::move(r); condition.notify_one(); // must notify under lock } }); @@ -122,8 +119,8 @@ T await(future x) { backoff *= 2) { { std::unique_lock lock{m}; - if (condition.wait_for(lock, backoff, [&] { return flag; })) { - break; + if (condition.wait_for(lock, backoff, [&] { return result.is_ready(); })) { + return detail::_get_ready_future{}(std::move(result)); } } detail::pts().wake(); // try to wake something to unstick. @@ -131,14 +128,11 @@ T await(future x) { #else - { - std::unique_lock lock{m}; - condition.wait(lock, [&] { return flag; }); - } + std::unique_lock lock{m}; + condition.wait(lock, [&] { return result.is_ready(); }); + return detail::_get_ready_future{}(std::move(result)); #endif - - return detail::_get_ready_future{}(std::move(result)); } namespace detail { diff --git a/stlab/concurrency/default_executor.hpp b/stlab/concurrency/default_executor.hpp index 63c7f432..be370f0c 100644 --- a/stlab/concurrency/default_executor.hpp +++ b/stlab/concurrency/default_executor.hpp @@ -19,7 +19,9 @@ #include #include #include +#include #include +#include #if STLAB_TASK_SYSTEM(LIBDISPATCH) #include @@ -94,12 +96,12 @@ struct executor_type { using result_type = void; template - auto operator()(F f) const -> std::enable_if_t> { - using f_t = decltype(f); + auto operator()(F&& f) const -> std::enable_if_t>> { + using f_t = std::decay_t; dispatch_group_async_f(detail::group()._group, dispatch_get_global_queue(platform_priority(P), 0), - new f_t(std::move(f)), [](void* f_) { + new f_t(std::forward(f)), [](void* f_) { auto f = static_cast(f_); (*f)(); delete f; @@ -161,12 +163,13 @@ class task_system { template void operator()(F&& f) { - auto work = CreateThreadpoolWork(&callback_impl, new F(std::forward(f)), - &_callBackEnvironment); + auto p = std::make_unique(std::forward(f)); + auto work = CreateThreadpoolWork(&callback_impl, p.get(), &_callBackEnvironment); if (work == nullptr) { throw std::bad_alloc(); } + p.release(); // ownership was passed to thread SubmitThreadpoolWork(work); } @@ -461,12 +464,13 @@ template struct executor_type { using result_type = void; - void operator()(task&& f) const { + template + auto operator()(F&& f) const -> std::enable_if_t>> { static task_system

only_task_system{[] { at_pre_exit([]() noexcept { only_task_system.join(); }); return task_system

{}; }()}; - only_task_system(std::move(f)); + only_task_system(std::forward(f)); } }; @@ -476,8 +480,9 @@ template struct executor_type { using result_type = void; - void operator()(task&& f) const { - pts().execute(P)>(std::move(f)); + template + auto operator()(F&& f) const -> std::enable_if_t>> { + pts().execute(P)>(std::forward(f)); } }; diff --git a/stlab/concurrency/future.hpp b/stlab/concurrency/future.hpp index b355a778..4b223a61 100644 --- a/stlab/concurrency/future.hpp +++ b/stlab/concurrency/future.hpp @@ -47,7 +47,6 @@ inline namespace v1 { enum class future_error_codes { // names for futures errors broken_promise = 1, - reduction_failed, no_state }; @@ -64,9 +63,6 @@ inline const char* Future_error_map( case future_error_codes::no_state: return "no state"; - case future_error_codes::reduction_failed: - return "reduction failed"; - default: return nullptr; } @@ -246,8 +242,8 @@ struct shared_future { template struct shared_task { virtual ~shared_task() = default; + virtual void remove_promise() = 0; - virtual void add_promise() = 0; virtual void operator()(Args... args) = 0; @@ -504,24 +500,20 @@ struct shared_base : std::enable_shared_from_this> { }; template -struct shared : shared_base, shared_task { +struct shared final : shared_base, shared_task { using function_t = task; - std::atomic_size_t _promise_count{1}; function_t _f; template shared(executor_t s, F&& f) : shared_base(std::move(s)), _f(std::forward(f)) {} void remove_promise() override { - if ((--_promise_count == 0) && _f) { - this->set_exception( - std::make_exception_ptr(future_error(future_error_codes::broken_promise))); - } + if (!_f) return; + this->set_exception( + std::make_exception_ptr(future_error(future_error_codes::broken_promise))); } - void add_promise() override { ++_promise_count; } - void operator()(Args... args) noexcept override { if (!_f) return; @@ -530,6 +522,7 @@ struct shared : shared_base, shared_task { } catch (...) { this->set_exception(std::current_exception()); } + _f = function_t(); } @@ -571,14 +564,10 @@ class packaged_task { if (auto p = _p.lock()) p->remove_promise(); } - packaged_task(const packaged_task& x) : _p(x._p) { - if (auto p = _p.lock()) p->add_promise(); - } + packaged_task(const packaged_task&) = delete; + packaged_task& operator=(const packaged_task&) = delete; packaged_task(packaged_task&&) noexcept = default; - - packaged_task& operator=(const packaged_task& x) { return *this = packaged_task{x}; } - packaged_task& operator=(packaged_task&& x) noexcept = default; template @@ -1182,9 +1171,9 @@ auto apply_when_any_arg(F& f, P& p) { } template -void attach_when_arg_(E&& executor, std::shared_ptr

& p, T a) { +void attach_when_arg_(E&& executor, std::shared_ptr

& shared, T a) { auto holds = - std::move(a).recover(std::forward(executor), [_w = std::weak_ptr

(p)](auto x) { + std::move(a).recover(std::forward(executor), [_w = std::weak_ptr

(shared)](auto x) { auto p = _w.lock(); if (!p) return; @@ -1194,8 +1183,8 @@ void attach_when_arg_(E&& executor, std::shared_ptr

& p, T a) { p->template done(std::move(x)); } }); - std::unique_lock lock{p->_guard}; - p->_holds[i] = std::move(holds); + std::unique_lock lock{shared->_guard}; + shared->_holds[i] = std::move(holds); } template @@ -1670,8 +1659,8 @@ auto async(E executor, F&& f, Args&&... args) executor, std::bind( [_f = std::forward(f)]( - unwrap_reference_t>&... args) mutable -> result_type { - return std::move(_f)(move_if>>(args)...); + unwrap_reference_t>&... brgs) mutable -> result_type { + return std::move(_f)(move_if>>(brgs)...); }, std::forward(args)...)); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 704fd998..8dc156dd 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -48,7 +48,8 @@ add_executable( stlab.test.future future_when_any_range_tests.cpp tuple_algorithm_test.cpp main.cpp - future_test_helper.hpp ) + future_test_helper.hpp + future_reduction_tests.cpp) if( NOT STLAB_NO_STD_COROUTINES ) target_sources( stlab.test.future PUBLIC future_coroutine_tests.cpp ) diff --git a/test/future_reduction_tests.cpp b/test/future_reduction_tests.cpp new file mode 100644 index 00000000..6972cff9 --- /dev/null +++ b/test/future_reduction_tests.cpp @@ -0,0 +1,25 @@ +/* + Copyright 2015 Adobe + Distributed under the Boost Software License, Version 1.0. + (See accompanying file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) +*/ + +/**************************************************************************************************/ + +#include + +#include + +#include + +using namespace stlab; + +BOOST_AUTO_TEST_SUITE(reduction_tests) + +BOOST_AUTO_TEST_CASE(void_void_reduction) { + auto f = make_ready_future(immediate_executor) | + [] { return make_ready_future(immediate_executor); }; + BOOST_REQUIRE(!f.exception()); +} + +BOOST_AUTO_TEST_SUITE_END()