From 0fbfbe3f10efd282119bdeee2df7ea94c8ad8cd2 Mon Sep 17 00:00:00 2001 From: Morgan Caron Date: Sun, 20 Sep 2026 13:12:17 +0200 Subject: [PATCH] fix(terminal): prevent race conditions and crash on canvas and scheduler teardown --- modules/Terminal/Area.mpp | 13 +++++++++++++ modules/Terminal/Canvas.mpp | 12 +++++++++--- modules/Terminal/Container.mpp | 13 +++++++++++++ modules/Terminal/Layout.mpp | 11 +++++++++++ modules/Terminal/Scrollable.mpp | 10 ++++++++++ modules/Terminal/Widget.mpp | 2 ++ modules/Thread/Scheduler.mpp | 20 +++++++++++++------- 7 files changed, 71 insertions(+), 10 deletions(-) diff --git a/modules/Terminal/Area.mpp b/modules/Terminal/Area.mpp index dda954eb..3f508394 100644 --- a/modules/Terminal/Area.mpp +++ b/modules/Terminal/Area.mpp @@ -20,9 +20,16 @@ export namespace CppUtils::Terminal m_viewport{(viewport.getSize().width() == 0 or viewport.getSize().height() == 0) ? Viewport{size} : viewport} {} + virtual inline ~Area() noexcept override + { + clearWidgetManager(); + } + template T> inline auto addWidget(this auto&& self [[clang::lifetimebound, msvc::lifetimebound]], std::unique_ptr widget) -> T& { + if (self.m_widget) + self.m_widget->clearWidgetManager(); self.m_widget = std::move(widget); if (self.hasWidgetManager()) self.m_widget->setWidgetManager(self.getWidgetManager()); @@ -42,6 +49,12 @@ export namespace CppUtils::Terminal m_widget->setWidgetManager(widgetManager); } + inline auto onWidgetManagerClear() noexcept -> void override + { + if (m_widget) + m_widget->clearWidgetManager(); + } + public: [[nodiscard]] inline auto getSize() const noexcept -> Container::Size2<> override { diff --git a/modules/Terminal/Canvas.mpp b/modules/Terminal/Canvas.mpp index a922198b..d7edebf4 100644 --- a/modules/Terminal/Canvas.mpp +++ b/modules/Terminal/Canvas.mpp @@ -16,6 +16,7 @@ import CppUtils.Terminal.CharAttributes; import CppUtils.Terminal.TextStyle; import CppUtils.Terminal.TextColor; import CppUtils.Terminal.BackgroundColor; +import CppUtils.Execution.EventDispatcher; export namespace CppUtils::Terminal { @@ -32,7 +33,7 @@ export namespace CppUtils::Terminal m_clearOnClose{clearOnClose}, m_previousBuffer{size} { - m_widgetManager.eventDispatcher.subscribe<"RequestUpdate">([this] { + m_updateSubscriptionIdentifier = m_widgetManager.eventDispatcher.subscribe<"RequestUpdate">([this] { print(); }); setWidgetManager(m_widgetManager); @@ -40,8 +41,13 @@ export namespace CppUtils::Terminal enableAnsi(); } - inline ~Canvas() noexcept + inline ~Canvas() noexcept override { + m_widgetManager.eventDispatcher.unsubscribe(m_updateSubscriptionIdentifier); + m_widgetManager.scheduler.cancelAll(); + clearWidgetManager(); + + auto lock = std::scoped_lock{m_printMutex}; if (m_clearOnClose and not m_firstPrint) { const auto terminalSize = getTerminalSize(); @@ -56,7 +62,6 @@ export namespace CppUtils::Terminal std::fflush(stdout); } m_widget.reset(); - clearWidgetManager(); } inline auto applyDifferences() noexcept -> void @@ -225,5 +230,6 @@ export namespace CppUtils::Terminal bool m_clearOnClose = false; DynamicAreaBuffer m_previousBuffer; WidgetManager m_widgetManager; + Execution::EventDispatcher::SubscriptionIdentifier m_updateSubscriptionIdentifier{}; }; } diff --git a/modules/Terminal/Container.mpp b/modules/Terminal/Container.mpp index bdf4b37d..03f7c988 100644 --- a/modules/Terminal/Container.mpp +++ b/modules/Terminal/Container.mpp @@ -32,9 +32,16 @@ export namespace CppUtils::Terminal addWidget(std::move(widget)); } + inline ~Container() noexcept override + { + clearWidgetManager(); + } + template T> inline auto addWidget(this auto&& self [[clang::lifetimebound, msvc::lifetimebound]], std::unique_ptr widget) -> T& { + if (self.m_widget) + self.m_widget->clearWidgetManager(); self.m_widget = std::move(widget); if (self.hasWidgetManager()) self.m_widget->setWidgetManager(self.getWidgetManager()); @@ -69,6 +76,12 @@ export namespace CppUtils::Terminal m_widget->setWidgetManager(widgetManager); } + inline auto onWidgetManagerClear() noexcept -> void override + { + if (m_widget) + m_widget->clearWidgetManager(); + } + inline auto onMouseEvent(const Mouse::Event& event) -> bool override { if (not m_widget) diff --git a/modules/Terminal/Layout.mpp b/modules/Terminal/Layout.mpp index 64082b19..f65efdc5 100644 --- a/modules/Terminal/Layout.mpp +++ b/modules/Terminal/Layout.mpp @@ -46,6 +46,11 @@ export namespace CppUtils::Terminal m_alignItems{alignItems} {} + inline ~Layout() noexcept override + { + clearWidgetManager(); + } + template T> inline auto addWidget(this auto&& self [[clang::lifetimebound, msvc::lifetimebound]], std::unique_ptr widget) -> T& { @@ -63,6 +68,12 @@ export namespace CppUtils::Terminal element->setWidgetManager(widgetManager); } + inline auto onWidgetManagerClear() noexcept -> void override + { + for (auto& element : m_elements) + element->clearWidgetManager(); + } + public: inline auto setDirection(this auto&& self, Direction direction) noexcept -> decltype(auto) { diff --git a/modules/Terminal/Scrollable.mpp b/modules/Terminal/Scrollable.mpp index 5ec3c4ce..29fbd152 100644 --- a/modules/Terminal/Scrollable.mpp +++ b/modules/Terminal/Scrollable.mpp @@ -37,6 +37,11 @@ export namespace CppUtils::Terminal m_fullContentArea{fullContentSize} {} + inline ~Scrollable() noexcept override + { + clearWidgetManager(); + } + template T> inline auto addWidget(std::unique_ptr widget) -> T& { @@ -49,6 +54,11 @@ export namespace CppUtils::Terminal m_fullContentArea.setWidgetManager(widgetManager); } + inline auto onWidgetManagerClear() noexcept -> void override + { + m_fullContentArea.clearWidgetManager(); + } + public: inline auto setScroll(this auto&& self, const Container::Size2<>& position) noexcept -> decltype(auto) { diff --git a/modules/Terminal/Widget.mpp b/modules/Terminal/Widget.mpp index 24dea859..958c317a 100644 --- a/modules/Terminal/Widget.mpp +++ b/modules/Terminal/Widget.mpp @@ -43,6 +43,7 @@ export namespace CppUtils::Terminal if (hasWidgetManager()) { getWidgetManager().scheduler.cancelOwner(this); + onWidgetManagerClear(); m_widgetManagerRef = std::nullopt; } } @@ -98,6 +99,7 @@ export namespace CppUtils::Terminal protected: virtual inline auto onWidgetManagerSet([[maybe_unused]] WidgetManager& widgetManager) noexcept -> void {} + virtual inline auto onWidgetManagerClear() noexcept -> void {} private: inline auto requestUpdateImpl(Thread::Scheduler::Clock::duration delay) -> void diff --git a/modules/Thread/Scheduler.mpp b/modules/Thread/Scheduler.mpp index 04f88cbb..4abff28d 100644 --- a/modules/Thread/Scheduler.mpp +++ b/modules/Thread/Scheduler.mpp @@ -83,7 +83,7 @@ export namespace CppUtils::Thread inline ~Scheduler() noexcept { - m_threadLoop.requestStop(); + m_threadLoop.stop(); cancelAll(); } @@ -136,6 +136,7 @@ export namespace CppUtils::Thread } tasksToRun.assign(std::ranges::begin(set), std::ranges::end(set)); + activeItems.insert(std::ranges::end(activeItems), std::ranges::begin(set), std::ranges::end(set)); processingTasks += std::ranges::size(set); map.clear(); set.clear(); @@ -161,6 +162,10 @@ export namespace CppUtils::Thread m_workCondition.notify_all(); m_finishedCondition.notify_all(); } + + for (auto& item : activeItems) + if (item->id == id) + item->cancelled = true; } inline auto cancelOwner(const void* owner) -> void @@ -205,6 +210,11 @@ export namespace CppUtils::Thread item->cancelled = true; map.clear(); set.clear(); + + m_finishedCondition.wait(accessor.getLockGuard(), [&] { + return std::ranges::empty(activeItems); + }); + m_workCondition.notify_all(); m_finishedCondition.notify_all(); } @@ -236,6 +246,7 @@ export namespace CppUtils::Thread if ((*it)->time <= endTime) { tasksToRun.push_back(*it); + activeItems.push_back(*it); map.erase((*it)->id); it = set.erase(it); ++accessor.value().processingTasks; @@ -255,15 +266,10 @@ export namespace CppUtils::Thread { for (auto&& item : tasks) { - { - auto accessor = m_items.access(); - accessor.value().activeItems.push_back(item); - } - try { m_threadPool.call([this, item] { - auto scope = Execution::ScopeGuard{[this, &item] { + auto scope = Execution::ScopeGuard{[this, item] { auto accessor = m_items.access(); std::erase(accessor.value().activeItems, item); m_finishedCondition.notify_all();