From e2e4756134de808d6ad44f7835f06c4678c51904 Mon Sep 17 00:00:00 2001 From: Aleksey Loginov Date: Thu, 30 May 2024 00:40:45 +0300 Subject: [PATCH 1/6] fix disposing order --- src/rpp/rpp/operators/take_until.hpp | 6 +++--- src/rpp/rpp/operators/timeout.hpp | 20 ++++++++++++-------- src/tests/rpp/test_take_until.cpp | 11 +++++++++++ 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/rpp/rpp/operators/take_until.hpp b/src/rpp/rpp/operators/take_until.hpp index 1cf382a11..9cf51d2ce 100644 --- a/src/rpp/rpp/operators/take_until.hpp +++ b/src/rpp/rpp/operators/take_until.hpp @@ -48,14 +48,14 @@ namespace rpp::operators::details void on_error(const std::exception_ptr& err) const { - state->dispose(); state->get_observer()->on_error(err); + state->dispose(); } void on_completed() const { - state->dispose(); state->get_observer()->on_completed(); + state->dispose(); } void set_upstream(const disposable_wrapper& d) { state->add(d); } @@ -69,8 +69,8 @@ namespace rpp::operators::details template void on_next(const T&) const { - take_until_observer_strategy_base::state->dispose(); take_until_observer_strategy_base::state->get_observer()->on_completed(); + take_until_observer_strategy_base::state->dispose(); } }; diff --git a/src/rpp/rpp/operators/timeout.hpp b/src/rpp/rpp/operators/timeout.hpp index 5143a2ce0..9ad1a1800 100644 --- a/src/rpp/rpp/operators/timeout.hpp +++ b/src/rpp/rpp/operators/timeout.hpp @@ -89,22 +89,26 @@ namespace rpp::operators::details void on_error(const std::exception_ptr& err) const noexcept { - const auto obs_with_timeout = disposable->get_observer_with_timeout_under_lock(); - if (disposable->is_disposed()) - return; + { + const auto obs_with_timeout = disposable->get_observer_with_timeout_under_lock(); + if (disposable->is_disposed()) + return; + obs_with_timeout->observer.on_error(err); + } disposable->dispose(); - obs_with_timeout->observer.on_error(err); } void on_completed() const noexcept { - const auto obs_with_timeout = disposable->get_observer_with_timeout_under_lock(); - if (disposable->is_disposed()) - return; + { + const auto obs_with_timeout = disposable->get_observer_with_timeout_under_lock(); + if (disposable->is_disposed()) + return; + obs_with_timeout->observer.on_completed(); + } disposable->dispose(); - obs_with_timeout->observer.on_completed(); } }; diff --git a/src/tests/rpp/test_take_until.cpp b/src/tests/rpp/test_take_until.cpp index f149c3d95..b833d7287 100644 --- a/src/tests/rpp/test_take_until.cpp +++ b/src/tests/rpp/test_take_until.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -224,3 +225,13 @@ TEST_CASE("take_until satisfies disposable contracts") test_operator_with_disposable(rpp::ops::take_until(rpp::source::never())); test_operator_with_disposable(rpp::ops::take_until(rpp::source::empty())); } + +TEST_CASE("take_until dispose after completion") +{ + std::vector log{}; + rpp::source::empty() + | rpp::ops::finally([&]() noexcept { log.push_back("finally"); }) + | rpp::ops::subscribe([](int) {}, [&log]() { log.push_back("completed"); }) + + CHECK(log == std::vector{"completed", "finally"}); +} From 0b77925b41618c17e24c54079da70b6d55058226 Mon Sep 17 00:00:00 2001 From: Aleksey Loginov Date: Thu, 30 May 2024 00:41:09 +0300 Subject: [PATCH 2/6] typo --- src/tests/rpp/test_take_until.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/rpp/test_take_until.cpp b/src/tests/rpp/test_take_until.cpp index b833d7287..e7c7007a4 100644 --- a/src/tests/rpp/test_take_until.cpp +++ b/src/tests/rpp/test_take_until.cpp @@ -231,7 +231,7 @@ TEST_CASE("take_until dispose after completion") std::vector log{}; rpp::source::empty() | rpp::ops::finally([&]() noexcept { log.push_back("finally"); }) - | rpp::ops::subscribe([](int) {}, [&log]() { log.push_back("completed"); }) + | rpp::ops::subscribe([](int) {}, [&log]() { log.push_back("completed"); }); - CHECK(log == std::vector{"completed", "finally"}); + CHECK(log == std::vector{"completed", "finally"}); } From 99b3512761234d96e05e12e12a7448f2dfd08eb0 Mon Sep 17 00:00:00 2001 From: Aleksey Loginov Date: Fri, 31 May 2024 09:26:29 +0000 Subject: [PATCH 3/6] options --- CMakeLists.txt | 2 +- cmake/dependencies.cmake | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5dbec6c2f..73a34961b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,7 +54,7 @@ if (RPP_BUILD_TESTS OR RPP_BUILD_BENCHMARKS) if (RPP_USE_LLVM_COV) add_test(NAME ${TARGET} COMMAND ${CMAKE_COMMAND} -E env LLVM_PROFILE_FILE=${RPP_TEST_RESULTS_DIR}/${TARGET}.profraw $ -v high) else() - add_test(NAME ${TARGET} COMMAND $ -v high) + add_test(NAME ${TARGET} COMMAND $ -v high -s) endif() endmacro() endif() diff --git a/cmake/dependencies.cmake b/cmake/dependencies.cmake index c7269919b..6430e41b4 100644 --- a/cmake/dependencies.cmake +++ b/cmake/dependencies.cmake @@ -6,12 +6,12 @@ macro(rpp_handle_3rdparty TARGET_NAME) target_compile_options(${TARGET_NAME} INTERFACE "-w") else() target_compile_options(${TARGET_NAME} PRIVATE "-w") + set_target_properties(${TARGET_NAME} PROPERTIES CXX_CLANG_TIDY "") + set_target_properties(${TARGET_NAME} PROPERTIES CXX_CPPCHECK "") + set_target_properties(${TARGET_NAME} PROPERTIES FOLDER 3rdparty) endif() set_target_properties(${TARGET_NAME} PROPERTIES INTERFACE_SYSTEM_INCLUDE_DIRECTORIES $) - set_target_properties(${TARGET_NAME} PROPERTIES CXX_CLANG_TIDY "") - set_target_properties(${TARGET_NAME} PROPERTIES CXX_CPPCHECK "") - set_target_properties(${TARGET_NAME} PROPERTIES FOLDER 3rdparty) endmacro() # ===================== SFML ======================= From 7c53bcd12e5ba1374707f7e59415c4fa671f18bf Mon Sep 17 00:00:00 2001 From: Aleksey Loginov Date: Fri, 31 May 2024 09:26:37 +0000 Subject: [PATCH 4/6] try to resolve deadlock --- src/rpp/rpp/operators/take_until.hpp | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/rpp/rpp/operators/take_until.hpp b/src/rpp/rpp/operators/take_until.hpp index 9cf51d2ce..99b9c9f93 100644 --- a/src/rpp/rpp/operators/take_until.hpp +++ b/src/rpp/rpp/operators/take_until.hpp @@ -31,12 +31,17 @@ namespace rpp::operators::details take_until_disposable(const TObserver& observer) : m_observer_with_mutex(observer) { - } + } + + void stop() { m_stopped = true; } + bool is_stopped() const { return m_stopped; } + bool stop_return_was_stopped() { return m_stopped.exchange(true); } rpp::utils::pointer_under_lock get_observer() { return m_observer_with_mutex; } private: rpp::utils::value_with_mutex m_observer_with_mutex{}; + std::atomic_bool m_stopped{}; }; template @@ -48,14 +53,14 @@ namespace rpp::operators::details void on_error(const std::exception_ptr& err) const { - state->get_observer()->on_error(err); - state->dispose(); + if (!state->stop_return_was_stopped()) + state->get_observer()->on_error(err); } void on_completed() const { - state->get_observer()->on_completed(); - state->dispose(); + if (!state->stop_return_was_stopped()) + state->get_observer()->on_completed(); } void set_upstream(const disposable_wrapper& d) { state->add(d); } @@ -69,8 +74,8 @@ namespace rpp::operators::details template void on_next(const T&) const { - take_until_observer_strategy_base::state->get_observer()->on_completed(); - take_until_observer_strategy_base::state->dispose(); + if (!take_until_observer_strategy_base::state->stop_return_was_stopped()) + take_until_observer_strategy_base::state->get_observer()->on_completed(); } }; @@ -80,7 +85,8 @@ namespace rpp::operators::details template void on_next(T&& v) const { - take_until_observer_strategy_base::state->get_observer()->on_next(std::forward(v)); + if (!take_until_observer_strategy_base::state->is_stopped()) + take_until_observer_strategy_base::state->get_observer()->on_next(std::forward(v)); } }; From e9b358e072fae07038856f678450795e9d5f7d8f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 31 May 2024 09:27:43 +0000 Subject: [PATCH 5/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/rpp/rpp/operators/take_until.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpp/rpp/operators/take_until.hpp b/src/rpp/rpp/operators/take_until.hpp index 99b9c9f93..ed2aca2c2 100644 --- a/src/rpp/rpp/operators/take_until.hpp +++ b/src/rpp/rpp/operators/take_until.hpp @@ -31,7 +31,7 @@ namespace rpp::operators::details take_until_disposable(const TObserver& observer) : m_observer_with_mutex(observer) { - } + } void stop() { m_stopped = true; } bool is_stopped() const { return m_stopped; } From 5837475a059e771da9cacdad573a32a7971912bb Mon Sep 17 00:00:00 2001 From: Aleksey Loginov Date: Fri, 31 May 2024 09:30:38 +0000 Subject: [PATCH 6/6] add more tests --- src/tests/rpp/test_take_until.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tests/rpp/test_take_until.cpp b/src/tests/rpp/test_take_until.cpp index e7c7007a4..7d65a96a8 100644 --- a/src/tests/rpp/test_take_until.cpp +++ b/src/tests/rpp/test_take_until.cpp @@ -224,6 +224,10 @@ TEST_CASE("take_until satisfies disposable contracts") { test_operator_with_disposable(rpp::ops::take_until(rpp::source::never())); test_operator_with_disposable(rpp::ops::take_until(rpp::source::empty())); + test_operator_finish_before_dispose(rpp::ops::take_until(rpp::source::empty())); + test_operator_over_observable_finish_before_dispose([](const auto& ob) { + return rpp::source::never() | rpp::ops::take_until(ob); + }); } TEST_CASE("take_until dispose after completion")