From 4011b555fe2edd0043bd48ca7a4a70a3448caa66 Mon Sep 17 00:00:00 2001 From: Chris Meyer <34664+cmeyer@users.noreply.github.com> Date: Thu, 14 Nov 2024 11:19:45 -0800 Subject: [PATCH 1/2] Fix several canvas item thread shutdown race conditions. --- launcher/DocumentWindow.cpp | 96 +++++++++++++++++++++++-------------- launcher/DocumentWindow.h | 1 + 2 files changed, 60 insertions(+), 37 deletions(-) diff --git a/launcher/DocumentWindow.cpp b/launcher/DocumentWindow.cpp index 00fbf2f..e6c1236 100644 --- a/launcher/DocumentWindow.cpp +++ b/launcher/DocumentWindow.cpp @@ -98,44 +98,44 @@ class RepaintManager { } - void requestRepaint(QWidget *window, PyCanvas *canvas) + void requestRepaint(PyCanvas *canvas) { QMutexLocker locker(&mutex); - for (const auto &p : requests) + for (const auto &r : requests) { - if (p.first == window && p.second == canvas) + if (r == canvas) return; } - requests.push_back(std::pair(window, canvas)); + requests.push_back(canvas); } void cancelRepaintRequest(PyCanvas *canvas) { QMutexLocker locker(&mutex); - std::list> new_requests; + std::list new_requests; - for (const auto &p : requests) + for (const auto &r : requests) { - if (p.second != canvas) - new_requests.push_back(p); + if (r != canvas) + new_requests.push_back(r); } requests = new_requests; } - void update(QWidget *widget) + void update() { // ideally only the passed widget could be updated; the widget may be a QDockWidget // which never calls update; so as a workaround, just update everything and clear the list. // this may be a problem (too many updates) in the future with multiple document windows. QMutexLocker locker(&mutex); - for (const auto &p : requests) + for (const auto &r : requests) { - p.second->update(); + r->update(); } requests.clear(); @@ -143,7 +143,7 @@ class RepaintManager private: QMutex mutex; - std::list> requests; + std::list requests; }; RepaintManager repaintManager; @@ -192,7 +192,7 @@ void DocumentWindow::timerEvent(QTimerEvent *event) { if (event->timerId() == m_periodic_timer && isVisible()) { - repaintManager.update(this); + repaintManager.update(); application()->dispatchPyMethod(m_py_object, "periodic", QVariantList()); } } @@ -2448,7 +2448,8 @@ CanvasSection::CanvasSection(int section_id, float device_pixel_ratio) */ PyCanvas::PyCanvas() - : m_pressed(false) + : m_closing(false) + , m_pressed(false) , m_grab_mouse_count(0) { setMouseTracking(true); @@ -2457,6 +2458,7 @@ PyCanvas::PyCanvas() PyCanvas::~PyCanvas() { + m_closing = true; // cancel any outstanding requests before shutting down the thread. repaintManager.cancelRepaintRequest(this); // now shut down the rendering thread by waiting until not rendering. @@ -2510,12 +2512,15 @@ void PyCanvas::continuePaintingSection(const RenderResult &render_result) section->record_latency = render_result.record_latency; auto pending_commands = section->m_pending_drawing_commands; section->m_pending_drawing_commands.reset(); - if (pending_commands) + // do not start a new task if closing. + if (!m_closing && pending_commands) { task = new PyCanvasRenderTask(this, section, pending_commands, section->m_device_pixel_ratio, section->m_rendered_timestamps); section->m_render_task = task; } - repaintManager.requestRepaint(window(), this); + // note: this may be occurring during a delete, in which case even the window may not be available. + if (!m_closing) + repaintManager.requestRepaint(this); } // launch the task outside of the mutex. @@ -3000,30 +3005,35 @@ void PyCanvas::setBinarySectionCommands(int section_id, const DrawingCommandsSha { QMutexLocker locker(&m_sections_mutex); - CanvasSectionSharedPtr section; - - if (m_sections.contains(section_id)) - { - section = m_sections[section_id]; - } - else + // this request can come in on a thread during shutdown and add a new request + // after the destructor has synced threading. so check if closing before proceeding. + if (!m_closing) { - auto screen = this->screen(); - auto device_pixel_ratio = screen ? screen->devicePixelRatio() : 1.0; // m_screen may be nullptr in earlier versions of Qt - section.reset(new CanvasSection(section_id, device_pixel_ratio)); - m_sections[section_id] = section; - } + CanvasSectionSharedPtr section; - pending_drawing_commands = section->m_pending_drawing_commands; + if (m_sections.contains(section_id)) + { + section = m_sections[section_id]; + } + else + { + auto screen = this->screen(); + auto device_pixel_ratio = screen ? screen->devicePixelRatio() : 1.0; // m_screen may be nullptr in earlier versions of Qt + section.reset(new CanvasSection(section_id, device_pixel_ratio)); + m_sections[section_id] = section; + } - if (!section->m_render_task) - { - task = new PyCanvasRenderTask(this, section, drawing_commands, section->m_device_pixel_ratio, section->m_rendered_timestamps); - section->m_render_task = task; - } - else - { - section->m_pending_drawing_commands = drawing_commands; + pending_drawing_commands = section->m_pending_drawing_commands; + + if (!section->m_render_task) + { + task = new PyCanvasRenderTask(this, section, drawing_commands, section->m_device_pixel_ratio, section->m_rendered_timestamps); + section->m_render_task = task; + } + else + { + section->m_pending_drawing_commands = drawing_commands; + } } } @@ -3035,6 +3045,18 @@ void PyCanvas::setBinarySectionCommands(int section_id, const DrawingCommandsSha void PyCanvas::removeSection(int section_id) { QMutexLocker locker(&m_sections_mutex); + + // ensure the section is not pending before removing. + auto section = m_sections[section_id]; + while (true) + { + if (!section->m_render_task) + break; + m_sections_mutex.unlock(); + QThread::msleep(1); + m_sections_mutex.lock(); + } + m_sections.remove(section_id); } diff --git a/launcher/DocumentWindow.h b/launcher/DocumentWindow.h index 739c430..0505769 100644 --- a/launcher/DocumentWindow.h +++ b/launcher/DocumentWindow.h @@ -648,6 +648,7 @@ class PyCanvas : public QWidget void continuePaintingSection(const RenderResult &render_result); private: + bool m_closing; QVariant m_py_object; QMutex m_sections_mutex; QMap m_sections; From 2fa38c139b509e547b1f9321b8b82f32aba749b5 Mon Sep 17 00:00:00 2001 From: Chris Meyer <34664+cmeyer@users.noreply.github.com> Date: Thu, 14 Nov 2024 14:26:02 -0800 Subject: [PATCH 2/2] Version 5.0.1. Fix canvas item thread crash. --- .github/workflows/recipe/meta.yaml | 2 +- CHANGES.rst | 4 ++++ launcher/CMakeLists.txt | 2 +- setup.py | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/recipe/meta.yaml b/.github/workflows/recipe/meta.yaml index f068397..27ba431 100644 --- a/.github/workflows/recipe/meta.yaml +++ b/.github/workflows/recipe/meta.yaml @@ -6,7 +6,7 @@ {% set win_exe = "NionUILauncher" %} {% set summary = "A native launcher for Nion UI apps." %} -{% set version = "5.0.0" %} +{% set version = "5.0.1" %} {% set name_u = name|replace('-', '_') %} diff --git a/CHANGES.rst b/CHANGES.rst index d9f471c..f8b8f32 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,6 +1,10 @@ Changelog (nionui-tool) ======================= +5.0.1 (2024-11-14) +------------------ +- Fix possible crash when closing canvas items. + 5.0.0 (2024-10-26) ------------------ - Eliminate dependence on NumPy for build. diff --git a/launcher/CMakeLists.txt b/launcher/CMakeLists.txt index 9beb316..e6fb062 100644 --- a/launcher/CMakeLists.txt +++ b/launcher/CMakeLists.txt @@ -19,7 +19,7 @@ option(USE_CONSOLE "Enable console" ON) set(APP_NAME "NionUILauncher") # app version -set(APP_VERSION "5.0.0") +set(APP_VERSION "5.0.1") set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/build) diff --git a/setup.py b/setup.py index 7b50712..76a484c 100644 --- a/setup.py +++ b/setup.py @@ -9,7 +9,7 @@ tool_id = "nionui" launcher = "NionUILauncher" -version = "5.0.0" +version = "5.0.1" def package_files(directory: str, prefix: str, prefix_drop: int) -> list[typing.Tuple[str, list[str]]]: