From f85755456a09484f7584cdfca54da4230e0c3d14 Mon Sep 17 00:00:00 2001 From: Amber Ehrlich Date: Sat, 13 Jul 2024 16:20:50 -0400 Subject: [PATCH] fix(coro): fix dpp::task's move assignment --- include/dpp/coro/awaitable.h | 2 +- include/dpp/coro/task.h | 29 +++++++++++++++++++---------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/include/dpp/coro/awaitable.h b/include/dpp/coro/awaitable.h index 4ba8ed79a9..f37bc93e66 100644 --- a/include/dpp/coro/awaitable.h +++ b/include/dpp/coro/awaitable.h @@ -459,7 +459,7 @@ class promise_base { value.template emplace<2>(std::move(ptr)); [[maybe_unused]] auto previous_value = this->state.fetch_or(sf_ready, std::memory_order::acq_rel); if constexpr (Notify) { - if (previous_value & sf_awaited) { + if ((previous_value & sf_awaited) != 0) { this->awaiter.resume(); } } diff --git a/include/dpp/coro/task.h b/include/dpp/coro/task.h index 8e6c862b3d..324c4957ca 100644 --- a/include/dpp/coro/task.h +++ b/include/dpp/coro/task.h @@ -106,6 +106,20 @@ class [[nodiscard("dpp::task cancels itself on destruction. use co_await on it, */ explicit task(handle_t handle_) : awaitable(&handle_.promise()), handle(handle_) {} + /** + * @brief Clean up our handle, cancelling any running task + */ + void cleanup() { + if (handle && this->valid()) { + if (this->abandon() & state_flags::sf_done) { + handle.destroy(); + } else { + cancel(); + } + handle = nullptr; + } + } + public: /** * @brief Default constructor, creates a task not bound to a coroutine. @@ -136,6 +150,7 @@ class [[nodiscard("dpp::task cancels itself on destruction. use co_await on it, */ task &operator=(task &&other) noexcept { awaitable::operator=(std::move(other)); + cleanup(); handle = std::exchange(other.handle, nullptr); return *this; } @@ -146,13 +161,7 @@ class [[nodiscard("dpp::task cancels itself on destruction. use co_await on it, * Destroys the handle. If the task is still running, it will be cancelled. */ ~task() { - if (handle && this->valid()) { - if (this->abandon() & state_flags::sf_done) { - handle.destroy(); - } else { - cancel(); - } - } + cleanup(); } /** @@ -406,14 +415,14 @@ std_coroutine::coroutine_handle<> final_awaiter::await_suspend(handle_t ha promise_t &promise = handle.promise(); uint8_t previous_state = promise.state.fetch_or(state_flags::sf_done); - if (previous_state & state_flags::sf_awaited) { // co_await-ed, resume parent - if (previous_state & state_flags::sf_broken) { // major bug, these should never be set together + if ((previous_state & state_flags::sf_awaited) != 0) { // co_await-ed, resume parent + if ((previous_state & state_flags::sf_broken) != 0) { // major bug, these should never be set together // we don't have a cluster so just log it on cerr std::cerr << "dpp: task promise ended in both an awaited and dangling state. this is a bug and a memory leak, please report it to us!" << std::endl; } return promise.release_awaiter(); } - if (previous_state & state_flags::sf_broken) { // task object is gone, free the handle + if ((previous_state & state_flags::sf_broken) != 0) { // task object is gone, free the handle handle.destroy(); } return std_coroutine::noop_coroutine();