Skip to content

Fix PRINT_REDIRECTOR calling into a destroyed LogViewer#593

Open
markomarkovic wants to merge 1 commit into
CadQuery:masterfrom
markomarkovic:logviewer-print-redirector
Open

Fix PRINT_REDIRECTOR calling into a destroyed LogViewer#593
markomarkovic wants to merge 1 commit into
CadQuery:masterfrom
markomarkovic:logviewer-print-redirector

Conversation

@markomarkovic

Copy link
Copy Markdown

Summary

PRINT_REDIRECTOR (a module-level singleton in main_window.py) connected a lambda to sigStdoutWrite for each MainWindow. The lambda was never disconnected, closed over self, and resolved self.components["log"] late through the dict MainMixin holds as a class attribute — so it reached whichever LogViewer registered last. Once a LogViewer was destroyed, the next print() called into a deleted C++ object and raised RuntimeError: wrapped C/C++ object of type LogViewer has been deleted.

Connecting the bound method self.components["log"].append instead lets PyQt drop the connection automatically when the LogViewer is destroyed.

Testing

Adds test_print_redirector_released_with_window: destroys a window's LogViewer, emits on the singleton, and asserts no exception reaches sys.excepthook. It fails on the old lambda and passes with the fix.

pytest tests/ — full suite passes.

The latent bug exists on master today; it just isn't exercised until multiple MainWindows are created and torn down (as the display-modes tests do).

Claude AI found and fixed the issue.

PRINT_REDIRECTOR is a module-level singleton, so the lambda connected to
sigStdoutWrite in prepare_panes accumulated one connection per MainWindow and
was never disconnected. Each lambda also closed over self and resolved
self.components["log"] late through a dict that MainMixin holds as a class
attribute, so it reached whichever LogViewer registered last. Once a LogViewer
was destroyed, the next print() called into a deleted C++ object and raised
RuntimeError.

Connecting the bound method instead lets PyQt drop the connection when the
receiver is destroyed.
@jmwright

Copy link
Copy Markdown
Member

@markomarkovic What are the steps to reproduce this error? I am not seeing it, even in CI.

@markomarkovic

Copy link
Copy Markdown
Author

Fair — it doesn't fail the suite as it stands, and that's really the point: the connection is dangling, but nothing currently exercises it. Two things hide it:

  1. PyQt routes an exception raised inside a slot to sys.excepthook instead of propagating it, so it surfaces as a stderr traceback rather than a failure.
  2. Nothing in the current tests destroys a LogViewer while PRINT_REDIRECTOR is still emitting.

Deterministic repro on master (6daf9c0) — fails there, passes with this PR:

import sys
from PyQt5 import sip
from PyQt5.QtWidgets import QMessageBox
from cq_editor.__main__ import MainWindow
from cq_editor.main_window import PRINT_REDIRECTOR


def test_stray_output_after_log_destroyed(qtbot, mocker):
    mocker.patch.object(QMessageBox, "question", return_value=QMessageBox.Yes)
    mocker.patch.object(QMessageBox, "warning", return_value=QMessageBox.Discard)

    win = MainWindow()
    qtbot.addWidget(win)
    sip.delete(win.components["log"])   # the window's LogViewer goes away

    errors = []
    original, sys.excepthook = sys.excepthook, lambda t, e, tb: errors.append(t.__name__)
    try:
        PRINT_REDIRECTOR.sigStdoutWrite.emit("stray output")
    finally:
        sys.excepthook = original

    assert errors == []   # master: ['RuntimeError'] - LogViewer has been deleted

Mechanism: PRINT_REDIRECTOR is a module-level singleton that outlives any window. PyQt drops a connection to a QObject's bound method when that QObject is destroyed, but a lambda connection has no receiver to track, so it survives and calls into freed C++.

Where it stops being theoretical: adding pytest-qt tests that build MainWindows. While writing tests for another feature, the full suite hit this repeatedly (once per previously-destroyed LogViewer) and pytest-qt escalated it into a SETUP ERROR on the following test. That's what prompted this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants