From a1f5d1bb9b04f810d7b8ec094e62b00fe1259da4 Mon Sep 17 00:00:00 2001 From: Aleksey Loginov Date: Tue, 30 Jan 2024 23:14:17 +0300 Subject: [PATCH] Auto dispose disposable in case of destruction if needed (#518) * add auto-dispose * handle refcount --- .../rpp/disposables/callback_disposable.hpp | 2 +- .../rpp/disposables/composite_disposable.hpp | 6 ++--- .../disposables/details/base_disposable.hpp | 7 ++--- .../rpp/disposables/disposable_wrapper.hpp | 2 +- src/rpp/rpp/disposables/fwd.hpp | 6 +++++ .../rpp/disposables/interface_disposable.hpp | 14 +++++++++- .../rpp/disposables/refcount_disposable.hpp | 8 +++--- .../rpp/observables/blocking_observable.hpp | 2 +- src/rpp/rpp/schedulers/new_thread.hpp | 2 +- src/rpp/rpp/schedulers/run_loop.hpp | 2 +- .../rpp/subjects/details/subject_state.hpp | 2 +- src/tests/rpp/test_disposables.cpp | 26 +++++++++---------- src/tests/utils/test_scheduler.hpp | 2 +- 13 files changed, 51 insertions(+), 30 deletions(-) diff --git a/src/rpp/rpp/disposables/callback_disposable.hpp b/src/rpp/rpp/disposables/callback_disposable.hpp index 646ecd730..99b921737 100644 --- a/src/rpp/rpp/disposables/callback_disposable.hpp +++ b/src/rpp/rpp/disposables/callback_disposable.hpp @@ -36,7 +36,7 @@ class callback_disposable final : public details::base_disposable } private: - void dispose_impl() noexcept override { std::move(m_fn)(); } // NOLINT(bugprone-exception-escape) + void base_dispose_impl(interface_disposable::Mode) noexcept override { std::move(m_fn)(); } // NOLINT(bugprone-exception-escape) private: Fn m_fn; diff --git a/src/rpp/rpp/disposables/composite_disposable.hpp b/src/rpp/rpp/disposables/composite_disposable.hpp index ee6130a56..a0e4980aa 100644 --- a/src/rpp/rpp/disposables/composite_disposable.hpp +++ b/src/rpp/rpp/disposables/composite_disposable.hpp @@ -39,7 +39,7 @@ class composite_disposable_impl : public interface_composite_disposable return m_current_state.load(std::memory_order::seq_cst) == State::Disposed; } - void dispose() noexcept final + void dispose_impl(interface_disposable::Mode mode) noexcept final { while (true) { @@ -47,7 +47,7 @@ class composite_disposable_impl : public interface_composite_disposable // need to acquire possible state changing from `add` if (m_current_state.compare_exchange_strong(expected, State::Disposed, std::memory_order::seq_cst)) { - dispose_impl(); + composite_dispose_impl(mode); m_disposables.dispose(); m_disposables.clear(); @@ -150,7 +150,7 @@ class composite_disposable_impl : public interface_composite_disposable } protected: - virtual void dispose_impl() noexcept {} + virtual void composite_dispose_impl(interface_disposable::Mode) noexcept {} private: enum class State : uint8_t diff --git a/src/rpp/rpp/disposables/details/base_disposable.hpp b/src/rpp/rpp/disposables/details/base_disposable.hpp index 9e89ad54d..35c896d58 100644 --- a/src/rpp/rpp/disposables/details/base_disposable.hpp +++ b/src/rpp/rpp/disposables/details/base_disposable.hpp @@ -31,15 +31,16 @@ class base_disposable_impl : public BaseInterface return m_disposed.load(std::memory_order::seq_cst); } - void dispose() noexcept final +private: + void dispose_impl(interface_disposable::Mode mode) noexcept final { // just need atomicity, not guarding anything if (m_disposed.exchange(true, std::memory_order::seq_cst) == false) - dispose_impl(); + base_dispose_impl(mode); } protected: - virtual void dispose_impl() noexcept = 0; + virtual void base_dispose_impl(interface_disposable::Mode mode) noexcept = 0; private: std::atomic_bool m_disposed{}; diff --git a/src/rpp/rpp/disposables/disposable_wrapper.hpp b/src/rpp/rpp/disposables/disposable_wrapper.hpp index 073b842c3..2580c158e 100644 --- a/src/rpp/rpp/disposables/disposable_wrapper.hpp +++ b/src/rpp/rpp/disposables/disposable_wrapper.hpp @@ -41,7 +41,7 @@ class auto_dispose_wrapper final ~auto_dispose_wrapper() noexcept { - // static_cast(m_data).dispose_impl(rpp::interface_disposable::Mode::Destroying); + static_cast(m_data).dispose_impl(rpp::interface_disposable::Mode::Destroying); } TDisposable* get() { return &m_data; } diff --git a/src/rpp/rpp/disposables/fwd.hpp b/src/rpp/rpp/disposables/fwd.hpp index 5963bbd65..7984ece18 100644 --- a/src/rpp/rpp/disposables/fwd.hpp +++ b/src/rpp/rpp/disposables/fwd.hpp @@ -12,6 +12,12 @@ #include +namespace rpp::details +{ +template +class auto_dispose_wrapper; +} + namespace rpp { struct interface_disposable; diff --git a/src/rpp/rpp/disposables/interface_disposable.hpp b/src/rpp/rpp/disposables/interface_disposable.hpp index a18cc3d01..3c7d4e069 100644 --- a/src/rpp/rpp/disposables/interface_disposable.hpp +++ b/src/rpp/rpp/disposables/interface_disposable.hpp @@ -33,6 +33,18 @@ struct interface_disposable * @brief Dispose disposable and free any underlying resources and etc. * @warning This function must be thread-safe */ - virtual void dispose() noexcept = 0; + void dispose() noexcept { dispose_impl(Mode::Disposing); } + + template + friend class rpp::details::auto_dispose_wrapper; + +protected: + enum class Mode : bool + { + Disposing = 0, // someone called "dispose" method manually + Destroying = 1 // called during destruction -> not needed to clear self in other disposables and etc + not allowed to call `shared_from_this` + }; + + virtual void dispose_impl(Mode mode) noexcept = 0; }; } \ No newline at end of file diff --git a/src/rpp/rpp/disposables/refcount_disposable.hpp b/src/rpp/rpp/disposables/refcount_disposable.hpp index 3d714cece..a83a35e63 100644 --- a/src/rpp/rpp/disposables/refcount_disposable.hpp +++ b/src/rpp/rpp/disposables/refcount_disposable.hpp @@ -45,7 +45,7 @@ class refcount_disposable : public rpp::details::enable_wrapper_from_this state) : m_state{std::move(state)} {} - void dispose_impl() noexcept override + void composite_dispose_impl(interface_disposable::Mode mode) noexcept override { - m_state.remove(this->wrapper_from_this()); + if (mode != interface_disposable::Mode::Destroying) + m_state.remove(this->wrapper_from_this()); + if (const auto locked = m_state.lock()) locked->release(); m_state = disposable_wrapper_impl::empty(); diff --git a/src/rpp/rpp/observables/blocking_observable.hpp b/src/rpp/rpp/observables/blocking_observable.hpp index f3e2163ca..446a9f799 100644 --- a/src/rpp/rpp/observables/blocking_observable.hpp +++ b/src/rpp/rpp/observables/blocking_observable.hpp @@ -28,7 +28,7 @@ class blocking_disposble final : public base_disposable m_cv.wait(lock, [this] { return m_completed; }); } - void dispose_impl() noexcept override + void base_dispose_impl(interface_disposable::Mode) noexcept override { { std::lock_guard lock{m_mutex}; diff --git a/src/rpp/rpp/schedulers/new_thread.hpp b/src/rpp/rpp/schedulers/new_thread.hpp index 81c48280f..fc33f96c2 100644 --- a/src/rpp/rpp/schedulers/new_thread.hpp +++ b/src/rpp/rpp/schedulers/new_thread.hpp @@ -64,7 +64,7 @@ class new_thread } private: - void dispose_impl() noexcept override + void base_dispose_impl(interface_disposable::Mode) noexcept override { if (!m_thread.joinable()) return; diff --git a/src/rpp/rpp/schedulers/run_loop.hpp b/src/rpp/rpp/schedulers/run_loop.hpp index 7220e79d8..6e1da4c99 100644 --- a/src/rpp/rpp/schedulers/run_loop.hpp +++ b/src/rpp/rpp/schedulers/run_loop.hpp @@ -89,7 +89,7 @@ class run_loop final return !m_queue.is_empty() && (m_queue.top()->is_disposed() || m_queue.top()->get_timepoint() <= now); } - void dispose_impl() noexcept override + void base_dispose_impl(interface_disposable::Mode) noexcept override { { std::lock_guard lock{m_mutex}; diff --git a/src/rpp/rpp/subjects/details/subject_state.hpp b/src/rpp/rpp/subjects/details/subject_state.hpp index 6dbd0636c..a7a591e51 100644 --- a/src/rpp/rpp/subjects/details/subject_state.hpp +++ b/src/rpp/rpp/subjects/details/subject_state.hpp @@ -90,7 +90,7 @@ class subject_state : public std::enable_shared_from_this> } private: - void dispose_impl() noexcept override + void composite_dispose_impl(interface_disposable::Mode) noexcept override { exchange_observers_under_lock_if_there(disposed{}); } diff --git a/src/tests/rpp/test_disposables.cpp b/src/tests/rpp/test_disposables.cpp index 25e9772a1..391b18b50 100644 --- a/src/tests/rpp/test_disposables.cpp +++ b/src/tests/rpp/test_disposables.cpp @@ -19,7 +19,7 @@ struct custom_disposable : public rpp::interface_disposable bool is_disposed() const noexcept final { return dispose_count > 1; } - void dispose() noexcept final { ++dispose_count; } + void dispose_impl(rpp::interface_disposable::Mode) noexcept final { ++dispose_count; } size_t dispose_count{}; }; @@ -134,18 +134,18 @@ TEMPLATE_TEST_CASE("disposable keeps state", "", rpp::details::disposables::dyna CHECK(other.is_disposed()); } } - // SECTION("disposable dispose on destruction") - // { - // { - // auto other = rpp::composite_disposable_wrapper::make(); - // CHECK(!other.is_disposed()); - // CHECK(!d.is_disposed()); - // other.add(d); - // CHECK(!other.is_disposed()); - // CHECK(!d.is_disposed()); - // } - // CHECK(d.is_disposed()); - // } + SECTION("disposable dispose on destruction") + { + { + auto other = rpp::composite_disposable_wrapper::make(); + CHECK(!other.is_disposed()); + CHECK(!d.is_disposed()); + other.add(d); + CHECK(!other.is_disposed()); + CHECK(!d.is_disposed()); + } + CHECK(d.is_disposed()); + } SECTION("add callback_disposable") { diff --git a/src/tests/utils/test_scheduler.hpp b/src/tests/utils/test_scheduler.hpp index 37f9cfd58..461ec7b60 100644 --- a/src/tests/utils/test_scheduler.hpp +++ b/src/tests/utils/test_scheduler.hpp @@ -56,7 +56,7 @@ class test_scheduler final } } - void dispose_impl() noexcept override {} + void base_dispose_impl(interface_disposable::Mode) noexcept override {} std::vector schedulings{}; std::vector executions{};