From dba0becc9efe0709bf1a6ad70c827d5b83fec2fa Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 02:01:10 -0300 Subject: [PATCH 01/15] fix(app): report an exited target once instead of spamming error dialogs Selecting a process that has already exited (or watching one exit with a tool window open) filled the screen with error dialogs until the app had to be killed from another terminal. The auto-refresh dialogs poll every 300-1000 ms and reported *every* failed poll with a modal. A modal runs a nested event loop, so the timer kept firing underneath it and the dialogs stacked - 332 of them in six seconds in a headless repro. `File -> Change Process...` fed the same loop from the other side: it closed the old handle while the Memory Map and the hex viewers were still polling through it. A failure is now reported only when it changes what the user sees: the first fetch (the window is empty) or the one that makes the poll give up after 3 s of continuous failure (the table is frozen from then on), and never twice for the same error. Failures in between heal quietly, which is what keeps a target failing on alternating ticks from stacking a modal per failure. On giving up, the poll stops and the status line says so - including after a filter keystroke rebuilds it. Changing process now tears down every dialog that held the old handle. Fixing that surfaced neighbouring teardown gaps that made the guarantee hollow: * `QDialog.done()` delivers no `QCloseEvent`, so the teardown that lived in `closeEvent` never ran for the dialogs' own Close button (`accept()`) or Esc (`reject()`). Every worker-bearing dialog kept polling, hidden and unreachable, and leaked a thread per open/close cycle. A `TearsDownOnClose` mixin now runs the teardown exactly once, whichever way a dialog is dismissed. * `CheatTable.shutdown()` joined its poller with a bounded wait and ignored the result, so a wedged poller kept reading while the handle it read through was closed and the widget owning its thread was deleted. * A detached worker (one that blew its join and was parked rather than stopped) could still be inside a read when the process handle was released - a recycled HANDLE on Windows, a deallocated Mach port on macOS. Releasing a handle now waits for those workers, and they all live in one registry instead of two, so nothing is invisible to that wait. * `application.main` released the handle the picker opened rather than the one the window ended up attached to, leaking the current target's handle after a process switch. Also documents that `get_threads` reports an exited process differently per platform (empty on Linux/Windows, `OSError` on macOS), unlike `get_memory_regions` and `get_modules`. Closes #74 --- PyMemoryEditor/app/_auto_refresh_dialog.py | 135 +++++++++- PyMemoryEditor/app/_widgets.py | 160 +++++++++++- PyMemoryEditor/app/application.py | 19 +- PyMemoryEditor/app/cheat_table.py | 31 ++- PyMemoryEditor/app/log_console_dialog.py | 7 +- PyMemoryEditor/app/main_window.py | 84 +++++-- PyMemoryEditor/app/memory_map_dialog.py | 15 +- PyMemoryEditor/app/memory_viewer_dialog.py | 7 +- PyMemoryEditor/app/modules_dialog.py | 15 +- PyMemoryEditor/app/open_process_dialog.py | 20 +- PyMemoryEditor/app/pointer_scan_dialog.py | 6 +- PyMemoryEditor/app/threads_dialog.py | 6 + PyMemoryEditor/process/abstract.py | 10 + docs/app.md | 18 ++ tests/app/test_auto_refresh_dialog.py | 277 +++++++++++++++++++++ tests/app/test_detached_worker_close.py | 179 +++++++++++++ 16 files changed, 924 insertions(+), 65 deletions(-) create mode 100644 tests/app/test_auto_refresh_dialog.py create mode 100644 tests/app/test_detached_worker_close.py diff --git a/PyMemoryEditor/app/_auto_refresh_dialog.py b/PyMemoryEditor/app/_auto_refresh_dialog.py index d49ea76..2a2919f 100644 --- a/PyMemoryEditor/app/_auto_refresh_dialog.py +++ b/PyMemoryEditor/app/_auto_refresh_dialog.py @@ -10,13 +10,34 @@ that self-throttles the poll, the detach-on-shutdown safety) where a fix to one copy wouldn't reach the others. This owns the lifecycle once; subclasses provide only what differs: how to fetch the data, and how to render it. + +It also owns *when a failure is worth interrupting the user*, which a polling +dialog cannot get wrong quietly: ``_on_data_failed`` is a modal, a modal runs a +nested event loop, and a poll left running underneath one stacks a dialog per +tick (issue #74). See :meth:`AutoRefreshTableDialog._handle_failed`. """ from typing import Callable, Optional -from PySide6.QtCore import QThread, QTimer, Signal +from PySide6.QtCore import QElapsedTimer, QThread, QTimer, Signal from PySide6.QtWidgets import QDialog -from ._widgets import shutdown_worker_thread +from ._widgets import TearsDownOnClose, shutdown_worker_thread + + +# How long a target may keep failing before the auto-refresh gives up. A +# failure is usually terminal — the target exited, or its handle was closed by +# File → Change Process — but not always: a module/thread enumeration can blip +# while the target loads images (``CreateToolhelp32Snapshot`` is documented to +# fail with ``ERROR_BAD_LENGTH`` mid-load), and giving up on the first blip +# would freeze a window that would have recovered on the next tick. +# +# Measured in wall-clock rather than ticks on purpose: "how long does the target +# get to recover" is the same question for every dialog, but the intervals are +# not (300 ms for Threads, 1000 ms for Memory Map / Modules), so a tick count +# would hand them budgets that differ by 3x. What gets *reported* is decided +# separately, in ``_handle_failed`` — this constant only bounds how long a +# failing target keeps a worker running. +_FAILURE_GRACE_MS = 3000 class _DataWorker(QThread): @@ -38,7 +59,7 @@ def run(self) -> None: self.ready.emit(data) -class AutoRefreshTableDialog(QDialog): +class AutoRefreshTableDialog(TearsDownOnClose, QDialog): """Owns the fetch-worker lifecycle + auto-refresh timer for a table dialog. Subclasses must: @@ -57,6 +78,18 @@ def __init__(self, process, *, refresh_interval_ms: int, parent=None): self._has_data = False self._refresh_interval_ms = refresh_interval_ms self._auto_timer: Optional[QTimer] = None + # Last error handed to ``_on_data_failed``, so a target that fails on + # every tick is reported once instead of once per tick. Cleared by a + # successful fetch or an explicit ``refresh()`` (see ``_handle_failed``). + self._last_error: Optional[str] = None + # Started on the first failure of a streak; None whenever the last + # fetch succeeded. Measures how long the target has been failing. + self._failing_since: Optional[QElapsedTimer] = None + # True from the moment the poll gives up until the next refresh(). Read + # by subclasses that rebuild their status line on user input (a filter + # keystroke), so the "stopped" note survives instead of being replaced + # by a count that looks live. + self._polling_gave_up = False def _start_auto_refresh(self) -> None: """Begin polling. Call once, after the first ``refresh()``. @@ -67,7 +100,7 @@ def _start_auto_refresh(self) -> None: """ self._auto_timer = QTimer(self) self._auto_timer.setInterval(self._refresh_interval_ms) - self._auto_timer.timeout.connect(self.refresh) + self._auto_timer.timeout.connect(self._auto_refresh_tick) self._auto_timer.start() # ------------------------------------------------------------------ # @@ -83,17 +116,50 @@ def _on_data_ready(self, data) -> None: raise NotImplementedError def _on_data_failed(self, message: str) -> None: - """Report a failed fetch. Runs on the UI thread.""" + """Report a failed fetch. Runs on the UI thread. + + Called only for a failure the user needs to see — the first fetch, or + the one that makes the poll give up — and never twice for the same + error (see :meth:`_handle_failed`), so a modal dialog is safe here. + """ raise NotImplementedError def _set_loading_hint(self) -> None: """Optionally show a one-time loading message before the first fetch.""" + def _on_polling_stopped(self) -> None: + """Note that the auto-refresh gave up. Runs on the UI thread. + + Fires once, when a target has been failing for ``_FAILURE_GRACE_MS`` + (see :meth:`_handle_failed`). The table is frozen on its last good data + from here on, so subclasses should say so — a silently stale window is + worse than an honest one. Optional. + """ + # ------------------------------------------------------------------ # # Lifecycle (shared) # ------------------------------------------------------------------ # def refresh(self) -> None: + """Fetch now, on the user's behalf. + + Explicit (re)fetch: it clears the error latch and restarts a poll that + repeated failures stopped, so reopening the dialog against a live target + picks the cadence back up. The timer itself goes through + :meth:`_auto_refresh_tick` instead, which must *not* re-arm. + """ + self._last_error = None + self._failing_since = None + self._polling_gave_up = False + if self._auto_timer is not None and not self._auto_timer.isActive(): + self._auto_timer.start() + self._start_fetch() + + def _auto_refresh_tick(self) -> None: + """Timer-driven fetch — never re-arms after a failure.""" + self._start_fetch() + + def _start_fetch(self) -> None: # Skip if a fetch is already in flight — the timer would otherwise stack # workers on a slow target. This self-throttles to the real fetch time. if self._worker is not None and self._worker.isRunning(): @@ -106,26 +172,79 @@ def refresh(self) -> None: worker = _DataWorker(self._fetch_data, self) worker.ready.connect(self._handle_ready) - worker.failed.connect(self._on_data_failed) + worker.failed.connect(self._handle_failed) worker.finished.connect(self._on_worker_finished) self._worker = worker worker.start() def _handle_ready(self, data) -> None: self._has_data = True + self._last_error = None + self._failing_since = None self._on_data_ready(data) + def _handle_failed(self, message: str) -> None: + """Handle a failed fetch: report it only when the user needs to know. + + Once the target is gone (or its handle was closed by File → Change + Process) *every* tick fails, and ``_on_data_failed`` pops a **modal** + dialog. A modal runs a nested event loop, so the timer fires again while + it is up and the failures stack — hundreds of focus-stealing dialogs the + user could only escape by killing the app from another TTY (issue #74). + + Reporting is therefore tied to the two moments where a failure actually + changes what the user sees: + + * the **first** fetch fails — the window is empty, so there is nothing + to look at but the error; + * the poll **gives up** after ``_FAILURE_GRACE_MS`` of continuous + failure — the table is frozen on stale data from here on. + + Everything in between heals quietly: the table keeps its last good rows + and the next tick usually recovers. That "quietly" is load-bearing, not + just tidy — reporting *every* failure re-opens the spam, because a + modal's nested loop lets the poll keep running underneath it, and a + target that fails on alternating ticks then stacks a fresh modal on each + failure until the interpreter runs out of stack. + """ + if self._failing_since is None: + self._failing_since = QElapsedTimer() + self._failing_since.start() + gave_up = False + else: + gave_up = ( + self._failing_since.elapsed() >= _FAILURE_GRACE_MS + and self._auto_timer is not None + and self._auto_timer.isActive() + ) + + if gave_up and self._auto_timer is not None: + self._auto_timer.stop() + self._polling_gave_up = True + + # ``_last_error`` doubles as "already reported": it keeps a repeat of the + # same error quiet, and its None-ness marks a window that has never + # reported anything (cleared by a successful fetch and by refresh()). + never_reported_yet = not self._has_data and self._last_error is None + if (gave_up or never_reported_yet) and message != self._last_error: + self._last_error = message + self._on_data_failed(message) + + # After the report, so the subclass's "stopped" note is what stays on + # screen once the user dismisses the modal. + if gave_up: + self._on_polling_stopped() + def _on_worker_finished(self) -> None: worker = self._worker self._worker = None if worker is not None: worker.deleteLater() - def closeEvent(self, event): # noqa: N802 — Qt naming + def _teardown(self) -> None: if self._auto_timer is not None: self._auto_timer.stop() # Unhook + join the worker; if it can't stop in time it's detached # rather than destroyed under us. shutdown_worker_thread(self._worker, wait_ms=1000) self._worker = None - super().closeEvent(event) diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index d0f5d1a..fae1316 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -8,7 +8,7 @@ from typing import Any, Callable, Iterable, List, Optional, Tuple -from PySide6.QtCore import Qt, QThread +from PySide6.QtCore import QElapsedTimer, Qt, QThread from PySide6.QtGui import QStandardItem @@ -52,19 +52,171 @@ def shutdown_worker_thread(worker: Optional[QThread], wait_ms: int = 2000) -> No worker.wait(wait_ms) if worker.isRunning(): - worker.setParent(None) - _DETACHED_WORKERS.append(worker) - worker.finished.connect(lambda: _reap_detached_worker(worker)) + detach_worker(worker) else: worker.deleteLater() +def detach_worker(worker: QThread) -> None: + """Park a worker that wouldn't stop, so its owner can be destroyed. + + Reparents it away from the dying widget and holds it in the module-level + registry until it finishes on its own. Callers that stop workers their own + way (``MainWindow._shutdown_worker`` pumps the event loop for its blocking + connections) must park them *here* rather than in a list of their own: + ``wait_for_detached_workers`` and ``call_when_detached_workers_finish`` + protect a process handle from exactly these threads, and a second registry + is a set of threads they cannot see. + """ + worker.setParent(None) + _DETACHED_WORKERS.append(worker) + worker.finished.connect(lambda: _reap_detached_worker(worker)) + + def _reap_detached_worker(worker: QThread) -> None: if worker in _DETACHED_WORKERS: _DETACHED_WORKERS.remove(worker) worker.deleteLater() +def wait_for_detached_workers(timeout_ms: int = 2000) -> bool: + """Block until no detached worker is running. Returns whether that happened. + + The blocking sibling of :func:`call_when_detached_workers_finish`, for + teardown that has no event loop left to deliver ``finished`` — application + exit, once ``app.exec()`` has returned. ``QThread.wait`` needs no loop, so + this is the only shape that works there. + + ``False`` means a worker is still wedged in a backend read after + ``timeout_ms``; the caller must then leave whatever that worker reads + through alone (at exit, the OS reclaims it moments later anyway). + """ + deadline_left = timeout_ms + for worker in list(_DETACHED_WORKERS): + try: + if not worker.isRunning(): + continue + elapsed = QElapsedTimer() + elapsed.start() + worker.wait(max(deadline_left, 0)) + deadline_left -= int(elapsed.elapsed()) + if worker.isRunning(): + return False + except RuntimeError: + # Already reaped and deleted between the snapshot and here. + continue + return True + + +def call_when_detached_workers_finish(callback: Callable[[], None]) -> None: + """Run ``callback`` once no currently-detached worker can still be running. + + Detaching (see :func:`shutdown_worker_thread`) keeps a wedged thread alive + *past* the dialog that owned it — which is the whole point, but it means the + thread may still be inside a backend read after its dialog is gone. Anything + that pulls the ground out from under such a read (closing the process handle + the worker reads through) has to wait for it, and this is how it waits + without blocking the UI thread. + + Runs ``callback`` synchronously when nothing is detached, which is the + normal case — detaching only happens when a worker blows its join timeout. + + The gate is the whole registry, not "the workers reading through *this* + handle" — the registry doesn't track what each worker reads, and being + over-conservative here costs a delayed close, while being wrong costs a + read-after-close. The price is that one permanently wedged worker strands + every later callback too: each subsequent process switch then leaks its old + handle for the life of the app. + + That is the deliberate trade — a leaked handle over a read-after-close — so + the callback must be something a leak can survive (releasing a handle, not + freeing memory the app needs back). Callers that cannot wait for an event + loop want :func:`wait_for_detached_workers`, which is bounded instead. + """ + pending = [worker for worker in _DETACHED_WORKERS if worker.isRunning()] + if not pending: + callback() + return + + remaining = {"count": len(pending), "called": False} + + def _one_finished() -> None: + remaining["count"] -= 1 + if remaining["count"] <= 0 and not remaining["called"]: + remaining["called"] = True + callback() + + for worker in pending: + fired = {"yet": False} + + def _slot(*_args, fired=fired) -> None: + # Idempotent: `finished` and the isFinished() poll below can both + # reach this, and only the first arrival may count. + if fired["yet"]: + return + fired["yet"] = True + _one_finished() + + try: + worker.finished.connect(_slot) + # The worker may have finished between the isRunning() filter and + # the connect above, in which case `finished` already fired and + # nothing would ever call the slot. Poll once to cover that window. + done = worker.isFinished() + except RuntimeError: + # Reaped and deleted from under us: it cannot be reading any more, + # and leaving it uncounted would strand the callback forever. + done = True + if done: + _slot() + + +class TearsDownOnClose: + """Mixin that runs a dialog's teardown on *every* way out, not just close. + + ``QDialog`` delivers a ``QCloseEvent`` only for a real close — the window + manager's button, or an explicit ``close()``. This app's own "Close" buttons + call ``accept()`` and Esc reaches ``reject()``; both land in ``done()``, + which hides the dialog and emits ``finished`` **without** any close event. + + A dialog that stops its timers and joins its worker in ``closeEvent`` is + therefore still polling after the user closed it — hidden, unreachable + (the main window drops its reference on ``finished``), leaking a thread per + open/close cycle, and able to pop a modal from behind nothing. Worse for + this app: its worker never reaches :func:`shutdown_worker_thread`, so it is + absent from the detached registry that keeps a process handle alive while + something is still reading through it. + + Subclasses implement :meth:`_teardown`; the mixin runs it exactly once, + whichever way the dialog is dismissed. It must precede ``QDialog`` in the + bases so its overrides win. + + Running *once* makes instances single-use, which is what every caller here + already assumes: the main window drops its reference on ``finished`` and + builds a fresh dialog next time. A dialog meant to be reshown after being + dismissed would have to re-arm what ``_teardown`` released rather than lean + on the latch. + """ + + def _teardown(self) -> None: + """Release timers and background threads. Runs exactly once.""" + raise NotImplementedError + + def _run_teardown_once(self) -> None: + if getattr(self, "_teardown_done", False): + return + self._teardown_done = True + self._teardown() + + def done(self, result: int) -> None: # noqa: N802 — Qt naming + self._run_teardown_once() + super().done(result) + + def closeEvent(self, event) -> None: # noqa: N802 — Qt naming + self._run_teardown_once() + super().closeEvent(event) + + class NumericItem(QStandardItem): """A QStandardItem that compares by its Qt.UserRole int payload. diff --git a/PyMemoryEditor/app/application.py b/PyMemoryEditor/app/application.py index 8841d4b..07cd645 100644 --- a/PyMemoryEditor/app/application.py +++ b/PyMemoryEditor/app/application.py @@ -513,6 +513,7 @@ def main(argv=None): from .open_process_dialog import OpenProcessDialog from ._icon import app_icon + from ._widgets import wait_for_detached_workers app = QApplication.instance() or QApplication(argv) app.setApplicationName("PyMemoryEditor") @@ -538,10 +539,20 @@ def main(argv=None): try: app.exec() finally: - try: - process.close() - except Exception: - pass + # A dialog fetch worker or the cheat poller that wouldn't stop in time + # was *detached* (see shutdown_worker_thread) and may still be reading + # through this handle. Closing it underneath one is a read-after-close, + # so wait for them first — and if one is still wedged, leave the handle + # to the OS, which reclaims it as this process exits anyway. There is no + # event loop left here, so this has to be the blocking wait rather than + # the signal-driven `call_when_detached_workers_finish`. + if wait_for_detached_workers(2000): + try: + # `process` is only the handle the picker opened; File → Change + # Process… released that one and attached the window to another. + window.process.close() + except Exception: + pass def main_cli(argv=None): diff --git a/PyMemoryEditor/app/cheat_table.py b/PyMemoryEditor/app/cheat_table.py index 88878ef..4627355 100644 --- a/PyMemoryEditor/app/cheat_table.py +++ b/PyMemoryEditor/app/cheat_table.py @@ -45,7 +45,7 @@ from PyMemoryEditor import AbstractProcess -from ._widgets import parse_hex_address +from ._widgets import parse_hex_address, shutdown_worker_thread from .cheat_entry import CheatEntry from .cheat_poll_worker import TICK_INTERVAL_MS, _CheatPollWorker from .value_types import VALUE_TYPES, ValueTypeSpec, find_spec, parse_value @@ -73,6 +73,9 @@ def __init__(self, process: AbstractProcess, parent=None): self._process = process self._entries: List[CheatEntry] = [] self._suspend_signals = False + # Set before _build_ui(), which connects cellChanged: the guards that + # read this flag sit on paths that signal can reach. + self._poller_stopped = False self._build_ui() @@ -101,12 +104,26 @@ def shutdown(self) -> None: ``closeEvent``, so ``_change_process`` and the top-level window's ``closeEvent`` need to drive the teardown explicitly to avoid leaving the poller running against a now-closed process handle. + + The join is *bounded*, and a tick only checks the stop flag between + iterations: draining queued writes and reading every cheat entry can + outlast the wait on a big table or a hung target. That case goes through + ``shutdown_worker_thread`` rather than being ignored, so the poller is + detached — kept alive away from this widget (which is about to be + deleted) and parked where ``call_when_detached_workers_finish`` can hold + the process handle open until it really is done reading. """ # Stop the publish timer first so no fresh snapshot races our shutdown. if self._publish_timer.isActive(): self._publish_timer.stop() + # Idempotent: shutdown() is driven from several places, and handing the + # same worker to shutdown_worker_thread twice would touch an object the + # detach machinery may already have deleted. + if self._poller_stopped: + return + self._poller_stopped = True self._poller.stop() - self._poller.wait(1000) + shutdown_worker_thread(self._poller, wait_ms=1000) def closeEvent(self, event): # noqa: N802 — Qt naming self.shutdown() @@ -317,6 +334,12 @@ def _on_cell_changed(self, row: int, column: int) -> None: # next poll tick reads the value back and corrects the cell if the # write didn't take, and an outright failure returns via # write_failed → _on_write_failed. + # + # Nothing to route it to once shutdown() has handed the worker over: + # it is either deleted or detached and finishing, and this table is + # on its way out with the process handle it was writing through. + if self._poller_stopped: + return self._poller.request_write( entry.address, entry.spec.pytype, entry.length, value ) @@ -327,6 +350,10 @@ def _on_cell_changed(self, row: int, column: int) -> None: def _publish_snapshot_to_worker(self) -> None: """Hand the worker a fresh immutable snapshot of every entry.""" + # shutdown() stops the timer that drives this, but a tick already queued + # when it ran would otherwise reach a worker that is being torn down. + if self._poller_stopped: + return snapshot = [ ( entry.address, diff --git a/PyMemoryEditor/app/log_console_dialog.py b/PyMemoryEditor/app/log_console_dialog.py index 380adc2..d5599b9 100644 --- a/PyMemoryEditor/app/log_console_dialog.py +++ b/PyMemoryEditor/app/log_console_dialog.py @@ -31,6 +31,8 @@ QVBoxLayout, ) +from ._widgets import TearsDownOnClose + _LOGGER_NAME = "PyMemoryEditor" @@ -71,7 +73,7 @@ def emit(self, record: logging.LogRecord) -> None: self.bridge.line_emitted.emit(line) -class LogConsoleDialog(QDialog): +class LogConsoleDialog(TearsDownOnClose, QDialog): """Live view of the PyMemoryEditor logger.""" # The dialog can be opened/closed repeatedly; we attach the handler on @@ -183,7 +185,7 @@ def _console_append(self, line: str) -> None: def _on_clear(self) -> None: self._console.clear() - def closeEvent(self, event): # noqa: N802 — Qt naming + def _teardown(self) -> None: # Detach the handler so the library doesn't keep emitting into a # dialog the user has dismissed. Don't lower the logger level past # what the caller had configured before us. @@ -195,4 +197,3 @@ def closeEvent(self, event): # noqa: N802 — Qt naming self._logger.removeHandler(self._handler) self._handler = None self._logger.setLevel(self._previous_level) - super().closeEvent(event) diff --git a/PyMemoryEditor/app/main_window.py b/PyMemoryEditor/app/main_window.py index bed4abe..bdca8a2 100644 --- a/PyMemoryEditor/app/main_window.py +++ b/PyMemoryEditor/app/main_window.py @@ -22,7 +22,7 @@ import psutil -from PySide6.QtCore import Qt, QEventLoop, QSettings, QThread, QTimer, Signal +from PySide6.QtCore import Qt, QEventLoop, QSettings, QTimer, Signal from PySide6.QtGui import QAction, QActionGroup, QCloseEvent, QKeySequence from PySide6.QtWidgets import ( QApplication, @@ -46,6 +46,7 @@ from PyMemoryEditor import AbstractProcess, __version__ from ._icon import app_icon +from ._widgets import call_when_detached_workers_finish, detach_worker from .application import DEFAULT_THEME_ID, THEMES, apply_theme from .cheat_entry import CheatEntry from .cheat_table import CheatTable @@ -81,9 +82,6 @@ def __init__(self, process: AbstractProcess): super().__init__() self._process = process self._worker: Optional[Union[FirstScanWorker, RefineScanWorker]] = None - # Workers that wouldn't stop in time on shutdown are detached here so - # the closing window can't destroy a still-running QThread. - self._zombie_workers: List[QThread] = [] self._region_snapshot: Optional[list] = None self._memory_map: Optional[MemoryMapDialog] = None self._hex_viewers: List[MemoryViewerDialog] = [] @@ -761,6 +759,17 @@ def _show_about(self) -> None: def _process_badge_text(self) -> str: return f"PID {self._process.pid} · {self._proc_name}" + @property + def process(self) -> AbstractProcess: + """The handle this window is currently attached to. + + Not necessarily the one it was constructed with: File → Change + Process… swaps it (and releases the old one), so anything outliving the + window — ``application.main``'s teardown — has to ask for it rather than + hold on to the handle it passed in. + """ + return self._process + def _window_title(self) -> str: # Qt prepends QGuiApplication.applicationDisplayName to the window # title on Windows/Linux; including it here would duplicate it. @@ -836,7 +845,6 @@ def _change_process(self) -> None: self._proc_create_time = self._read_proc_create_time() self.setWindowTitle(self._window_title()) self._process_badge.setText(self._process_badge_text()) - self._region_snapshot = None self._results_model.clear() self._scanner.set_has_results(False) # If the previous target exited, _check_process_alive locked the scanner @@ -845,8 +853,12 @@ def _change_process(self) -> None: self._scanner.set_busy(False) # Tear down auxiliary dialogs that hold a reference to the old - # process — reopening them rebuilds against the new target. + # process — reopening them rebuilds against the new target. The Memory + # Map belongs here too: it auto-refreshes on a timer, so leaving it open + # left it polling the handle closed just below — one of the two paths + # into the error-dialog spam of issue #74. for dialog_attr in ( + "_memory_map", "_threads_dialog", "_modules_dialog", "_pointer_chain_dialog", @@ -856,6 +868,20 @@ def _change_process(self) -> None: if existing is not None: existing.close() setattr(self, dialog_attr, None) + + # Hex viewers keep their own auto-refresh timer against the old handle. + # Each is WA_DeleteOnClose and drops itself from this list on `destroyed` + # — which fires when the deleteLater is processed, i.e. after this method + # returns. So the copy is belt-and-braces and the clear() is what + # actually empties the list; neither is redundant. + for viewer in list(self._hex_viewers): + viewer.close() + self._hex_viewers.clear() + + # Cleared *after* the teardown above: closing the Memory Map emits + # ``finished``, and its handler adopts that dialog's last snapshot as + # the cached one — which belongs to the process we just left. + self._region_snapshot = None # Replace the cheat table — old entries point at the previous process. # QSplitter has no QLayout, so we use its native replaceWidget(index). old_cheat = self._cheat @@ -867,11 +893,21 @@ def _change_process(self) -> None: old_cheat.shutdown() except Exception: pass - # The old poller is joined now, so it's safe to release the old handle. - try: - old_process.close() - except Exception: - pass + # The old poller has been stopped, but nothing here can *force* a thread + # wedged in a backend read to return: both the cheat poller and the + # dialog fetch workers fall back to being detached when they blow their + # join. Releasing the handle under one of those is a read-after-close (a + # recycled HANDLE on Windows, a deallocated Mach port on macOS), so hand + # the close to the helper that waits for every detached worker. Nothing + # detached — the normal case — closes inline. + + def _release_old_handle(process=old_process) -> None: + try: + process.close() + except Exception: + pass + + call_when_detached_workers_finish(_release_old_handle) old_index = self._right_splitter.indexOf(old_cheat) self._cheat = CheatTable(self._process) self._cheat.pointer_scan_for_address.connect(self._open_pointer_scan_dialog) @@ -915,6 +951,15 @@ def closeEvent(self, event: QCloseEvent) -> None: if dialog is not None: dialog.close() + # Hex viewers run the same kind of read worker and are children of this + # window, so destroying it would take a running QThread down with it. + # No clear() here (unlike _change_process, which keeps running against a + # new target afterwards): this window is on its way out with the list. + # (The Pointer Chain dialog is deliberately absent from both lists: it + # resolves synchronously and starts no thread.) + for viewer in list(self._hex_viewers): + viewer.close() + self.closing.emit() super().closeEvent(event) @@ -968,20 +1013,15 @@ def _shutdown_worker(self) -> None: if worker.isRunning(): # Still wedged in a backend call. Detach it from this window so the - # imminent window destruction can't destroy a live QThread, and - # keep it referenced until it finishes on its own. - worker.setParent(None) - self._zombie_workers.append(worker) - worker.finished.connect(lambda: self._reap_worker(worker)) + # imminent window destruction can't destroy a live QThread, and keep + # it referenced until it finishes on its own. It goes in the shared + # registry, not a list of our own: a scan worker still reading is + # precisely what must hold the process handle open in + # _change_process and at app exit. + detach_worker(worker) else: worker.deleteLater() - def _reap_worker(self, worker: QThread) -> None: - """Drop our reference to a detached worker once it has finished.""" - if worker in self._zombie_workers: - self._zombie_workers.remove(worker) - worker.deleteLater() - def _safe_for_json(value) -> object: if value is None or isinstance(value, (str, int, float, bool)): diff --git a/PyMemoryEditor/app/memory_map_dialog.py b/PyMemoryEditor/app/memory_map_dialog.py index ee28435..a0be6bf 100644 --- a/PyMemoryEditor/app/memory_map_dialog.py +++ b/PyMemoryEditor/app/memory_map_dialog.py @@ -414,15 +414,20 @@ def _populate(self) -> None: self._table.setSortingEnabled(True) if needle: - self._count_label.setText( + count_text = ( f"{shown:,} of {total:,} regions shown · " f"{_format_size(total_bytes)} mapped" ) else: - self._count_label.setText( + count_text = ( f"{total:,} regions · " f"{_format_size(total_bytes)} of virtual address space mapped" ) + # A filter keystroke rebuilds this line long after the poll gave up — + # without the suffix the stale table would look live again. + if self._polling_gave_up: + count_text += " · auto-refresh stopped" + self._count_label.setText(count_text) # Restore the view. A just-allocated region wins and is scrolled into # view; otherwise re-select whatever was selected before and keep the @@ -451,6 +456,12 @@ def _on_data_failed(self, message: str) -> None: self, "Memory Map", f"Failed to read memory regions:\n\n{message}" ) + def _on_polling_stopped(self) -> None: + self._count_label.setText( + "Failed to read memory regions — auto-refresh stopped. " + "Close and reopen this window to retry." + ) + def _selected_region(self) -> Optional[MemoryRegion]: rows = self._table.selectionModel().selectedRows() if not rows: diff --git a/PyMemoryEditor/app/memory_viewer_dialog.py b/PyMemoryEditor/app/memory_viewer_dialog.py index dfe37f3..83e5be9 100644 --- a/PyMemoryEditor/app/memory_viewer_dialog.py +++ b/PyMemoryEditor/app/memory_viewer_dialog.py @@ -25,7 +25,7 @@ from PyMemoryEditor import AbstractProcess from ._auto_refresh_dialog import _DataWorker -from ._widgets import parse_hex_address, shutdown_worker_thread +from ._widgets import TearsDownOnClose, parse_hex_address, shutdown_worker_thread # Child of the "PyMemoryEditor" logger, so the Log Console (which attaches a @@ -47,7 +47,7 @@ def _format_hex_dump(base: int, data: bytes) -> str: return "\n".join(lines) -class MemoryViewerDialog(QDialog): +class MemoryViewerDialog(TearsDownOnClose, QDialog): """Hex viewer + auto-refresh, with a "write bytes back" button.""" def __init__( @@ -276,11 +276,10 @@ def _write_bytes(self) -> None: self._status.setText(f"Wrote {len(data)} bytes to 0x{addr:X}.") self.refresh() - def closeEvent(self, event) -> None: + def _teardown(self) -> None: self._timer.stop() # Unhook + join the read worker; if it's still wedged in a backend call # it's detached rather than destroyed under us (same safety the # auto-refresh dialogs use). shutdown_worker_thread(self._worker, wait_ms=1000) self._worker = None - super().closeEvent(event) diff --git a/PyMemoryEditor/app/modules_dialog.py b/PyMemoryEditor/app/modules_dialog.py index 3254778..b5b85e9 100644 --- a/PyMemoryEditor/app/modules_dialog.py +++ b/PyMemoryEditor/app/modules_dialog.py @@ -190,9 +190,14 @@ def _apply_filter(self) -> None: total = len(self._modules) if needle: - self._count_label.setText(f"{shown:,} of {total:,} module(s) shown") + count_text = f"{shown:,} of {total:,} module(s) shown" else: - self._count_label.setText(f"{total:,} module(s)") + count_text = f"{total:,} module(s)" + # A filter keystroke rebuilds this line long after the poll gave up — + # without the suffix the stale table would look live again. + if self._polling_gave_up: + count_text += " · auto-refresh stopped" + self._count_label.setText(count_text) # Restore the user's selection + scroll, so the periodic refresh doesn't # clear what they had highlighted or jump the table around. @@ -214,6 +219,12 @@ def _on_data_failed(self, message: str) -> None: self, "Modules", f"Failed to enumerate modules:\n\n{message}" ) + def _on_polling_stopped(self) -> None: + self._count_label.setText( + "Failed to enumerate modules — auto-refresh stopped. " + "Close and reopen this window to retry." + ) + def _selected_module(self) -> Optional[dict]: rows = self._table.selectionModel().selectedRows() if not rows: diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index e58fae4..0dbb9c7 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -39,7 +39,7 @@ ) from ._icon import app_icon -from ._widgets import NumericItem +from ._widgets import NumericItem, TearsDownOnClose, shutdown_worker_thread if sys.platform == "win32": @@ -161,7 +161,7 @@ def lessThan(self, left: QModelIndex, right: QModelIndex) -> bool: # noqa: N802 return left_item < right_item -class OpenProcessDialog(QDialog): +class OpenProcessDialog(TearsDownOnClose, QDialog): """Process picker. Returns the opened ``AbstractProcess`` via ``.process``.""" COL_PID = 0 @@ -327,16 +327,14 @@ def _on_scan_finished(self) -> None: if worker is not None: worker.deleteLater() - def closeEvent(self, event): # noqa: N802 — Qt naming + def _teardown(self) -> None: self._refresh_timer.stop() - if self._scan_worker is not None and self._scan_worker.isRunning(): - try: - self._scan_worker.rows_ready.disconnect() - self._scan_worker.finished.disconnect() - except (RuntimeError, TypeError): - pass - self._scan_worker.wait(1000) - super().closeEvent(event) + # Hand the enumeration worker to the shared shutdown: it disconnects, + # joins, and — when psutil is wedged on a huge process list — detaches + # instead of leaving a live QThread parented to a dialog that is about + # to be dropped (destroying one aborts the process). + shutdown_worker_thread(self._scan_worker, wait_ms=1000) + self._scan_worker = None def _on_filter_changed(self, text: str) -> None: self._proxy.setFilterFixedString(text) diff --git a/PyMemoryEditor/app/pointer_scan_dialog.py b/PyMemoryEditor/app/pointer_scan_dialog.py index 70db079..d16e4bb 100644 --- a/PyMemoryEditor/app/pointer_scan_dialog.py +++ b/PyMemoryEditor/app/pointer_scan_dialog.py @@ -55,6 +55,7 @@ from ._widgets import ( MONOSPACE_FAMILY, NumericItem, + TearsDownOnClose, parse_hex_address, shutdown_worker_thread, ) @@ -319,7 +320,7 @@ def _resolve_and_read( _COL_VALUE = 5 -class PointerScanDialog(QDialog): +class PointerScanDialog(TearsDownOnClose, QDialog): """Reverse pointer scan with a Cheat-Engine-style result table.""" # qulonglong: 64-bit addresses overflow Qt's signed-32-bit default. @@ -997,10 +998,9 @@ def _on_compare_saved(self) -> None: ) self._export_btn.setEnabled(True) - def closeEvent(self, event): # noqa: N802 — Qt naming + def _teardown(self) -> None: # Unhook every signal (the previous code missed `error`/`finished`, so # a late emit could land on a destroyed dialog) and join the worker; if # it can't stop in time it's detached rather than destroyed under us. shutdown_worker_thread(self._worker, wait_ms=2000) self._worker = None - super().closeEvent(event) diff --git a/PyMemoryEditor/app/threads_dialog.py b/PyMemoryEditor/app/threads_dialog.py index 5c6694f..495ff77 100644 --- a/PyMemoryEditor/app/threads_dialog.py +++ b/PyMemoryEditor/app/threads_dialog.py @@ -169,3 +169,9 @@ def _on_data_failed(self, message: str) -> None: QMessageBox.critical( self, "Threads", f"Failed to enumerate threads:\n\n{message}" ) + + def _on_polling_stopped(self) -> None: + self._count_label.setText( + "Failed to enumerate threads — auto-refresh stopped. " + "Close and reopen this window to retry." + ) diff --git a/PyMemoryEditor/process/abstract.py b/PyMemoryEditor/process/abstract.py index 35ff1b8..68a6df8 100644 --- a/PyMemoryEditor/process/abstract.py +++ b/PyMemoryEditor/process/abstract.py @@ -227,6 +227,16 @@ def get_threads(self) -> Generator[ThreadInfo, None, None]: The ``tid`` field's *meaning* is platform-specific (POSIX TID on Linux, DWORD TID on Windows, Mach port name on macOS). + A process that has exited is *not* reported uniformly: Linux and + Windows enumerate nothing and yield an empty sequence (their sources — + ``/proc//task`` and a system-wide thread snapshot filtered by pid — + simply come up empty), while macOS raises ``OSError`` because + ``task_threads`` fails on a dead task. Don't read "no threads" as "still + running": check liveness separately if that distinction matters. + Contrast :meth:`get_memory_regions` and :meth:`get_modules`, which do + raise :class:`~PyMemoryEditor.ProcessIDNotExistsError` on every + platform. + Use :attr:`main_thread` for the conventional "main thread" shortcut. """ raise NotImplementedError() diff --git a/docs/app.md b/docs/app.md index 46a68c6..2f4fb72 100644 --- a/docs/app.md +++ b/docs/app.md @@ -87,6 +87,24 @@ from the **Theme** button on the toolbar; your choice is remembered between runs 6. (Optional) **Run a Pointer Scan** on the result to find a chain that survives restarts. +## When the target exits + +The app polls the target while you work — the Memory Map, Modules and Threads +windows re-read it on a timer (a few times a second for Threads), and the main +window checks every couple of seconds that it is still alive. + +When the process goes away (or you switch to another one via +`File → Change Process…`), you get **one** notice per window rather than one per +poll: the main window tells you the target exited, and any open auxiliary window +reports the failure once, stops refreshing, and says so in its status line +(`auto-refresh stopped`). Its table stays on the last data it read, so you can +still copy an address out of it. Close and reopen the window to point it at a +live target again. + +Brief hiccups don't trigger any of that — a read that fails and recovers within +a few seconds is left alone, so a busy target loading libraries won't interrupt +you. + ## Importing & exporting The Cheat Table and Pointer Scan results are stored as plain **JSON**, so you diff --git a/tests/app/test_auto_refresh_dialog.py b/tests/app/test_auto_refresh_dialog.py new file mode 100644 index 0000000..3597e26 --- /dev/null +++ b/tests/app/test_auto_refresh_dialog.py @@ -0,0 +1,277 @@ +# -*- coding: utf-8 -*- + +""" +Tests for ``PyMemoryEditor/app/_auto_refresh_dialog.py``. + +Regression cover for issue #74: the Memory Map / Modules / Threads dialogs poll +the target every 300–1000 ms and report a failed fetch with a *modal* dialog. +Once the target exits (or its handle is closed by File → Change Process) every +tick fails, and because a modal runs a nested event loop the timer keeps firing +while it's up — the app ended up with hundreds of stacked, focus-stealing error +boxes and had to be killed from another terminal. + +The contract now: a failure is reported only when it changes what the user sees +— the first fetch (empty window) or the one that makes the poll give up after +``_FAILURE_GRACE_MS`` of continuous failure (frozen table) — and never twice for +the same error. Failures in between heal quietly, which is what keeps a target +failing on alternating ticks from stacking a modal per failure. + +Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via +the ``app`` extra). +""" + +import os +import time + +import pytest + + +pytest.importorskip("PySide6", reason="App tests require PySide6 (install with [app] extra).") + +# Offscreen platform plugin: no display server needed, runs on CI. +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + + +@pytest.fixture(scope="module") +def qapp(): + """A single QApplication for the module (Qt allows only one per process).""" + from PySide6.QtWidgets import QApplication + + return QApplication.instance() or QApplication([]) + + +def _spin(qapp, milliseconds: int) -> None: + """Pump the event loop for a while, so timers fire and workers land.""" + from PySide6.QtCore import QElapsedTimer + + elapsed = QElapsedTimer() + elapsed.start() + while elapsed.elapsed() < milliseconds: + qapp.processEvents() + time.sleep(0.005) + + +@pytest.fixture +def short_grace(monkeypatch): + """Shrink the give-up window so the timing tests stay sub-second.""" + from PyMemoryEditor.app import _auto_refresh_dialog + + monkeypatch.setattr(_auto_refresh_dialog, "_FAILURE_GRACE_MS", 150) + return 150 + + +@pytest.fixture +def make_dialog(qapp): + """Factory for an ``AutoRefreshTableDialog`` whose fetch can be made to fail. + + The dialogs are parentless here, so PySide6 destroys the C++ object as soon + as the Python reference goes away. The fixture closes them (joining the + fetch worker) and pumps the loop once more, so no live QThread is left + hanging off an object about to be deleted. + """ + from PyMemoryEditor.app._auto_refresh_dialog import AutoRefreshTableDialog + + class _Dialog(AutoRefreshTableDialog): + def __init__(self, interval_ms, failing): + super().__init__(object(), refresh_interval_ms=interval_ms) + self.failing = failing + self.flapping = False + self.fetches = 0 + self.errors = [] + self.rendered = 0 + self.gave_up = 0 + + def _fetch_data(self): + # Runs on the worker thread, like the real enumerations. + self.fetches += 1 + if self.flapping: + self.failing = not self.failing + if self.failing: + raise RuntimeError('The process ID "4242" does not exist.') + return ["ok"] + + def _on_data_ready(self, data): + self.rendered += 1 + + def _on_data_failed(self, message): + # Stands in for the subclasses' QMessageBox.critical. + self.errors.append(message) + + def _on_polling_stopped(self): + # Stands in for the subclasses' "auto-refresh stopped" label. + self.gave_up += 1 + + dialogs = [] + + def _factory(interval_ms=50, failing=False): + dialog = _Dialog(interval_ms, failing) + dialog.refresh() + dialog._start_auto_refresh() + _spin(qapp, 100) # let the first fetch land + dialogs.append(dialog) + return dialog + + yield _factory + + for dialog in dialogs: + dialog.close() + _spin(qapp, 50) + + +def test_a_target_that_dies_is_reported_once(qapp, short_grace, make_dialog): + """A target that fails on every tick must produce a single error report.""" + dialog = make_dialog() + assert dialog.errors == [] + assert dialog.rendered > 0 + + dialog.failing = True + _spin(qapp, 600) + + # Without the fix this grew by one modal per 50 ms tick — and because each + # modal runs a nested event loop, the timer kept firing and they stacked. + assert len(dialog.errors) == 1 + assert "does not exist" in dialog.errors[0] + + +def test_polling_stops_when_failures_persist(qapp, short_grace, make_dialog): + """A dead target isn't coming back under the same handle — stop polling.""" + dialog = make_dialog() + dialog.failing = True + _spin(qapp, 600) + + assert dialog._auto_timer is not None and not dialog._auto_timer.isActive() + fetches = dialog.fetches + _spin(qapp, 300) + # Bounded work: it gives up instead of burning a worker per tick forever. + assert dialog.fetches == fetches + # And it says so exactly once, so the frozen table isn't silently stale. + assert dialog.gave_up == 1 + + +def test_a_flapping_target_never_stacks_dialogs(qapp, make_dialog): + """Fail / succeed / fail / succeed must stay silent, not pop a modal a tick. + + The regression this guards: reporting every failure looked safe because the + same error is never reported twice in a row — but each success cleared that + latch, so an alternating target popped a modal per failure, each one nesting + an event loop inside the previous modal until the stack blew up. + """ + dialog = make_dialog() + dialog.flapping = True + _spin(qapp, 800) + + assert dialog.errors == [] + assert dialog.gave_up == 0 + assert dialog._auto_timer.isActive() # a healing target keeps polling + assert dialog.rendered > 1 # and keeps rendering its good ticks + + +def test_the_first_fetch_failure_is_reported_immediately(qapp, make_dialog): + """Nothing to look at but the error, so don't sit on it for the grace window.""" + dialog = make_dialog(failing=True) # broken before the window ever filled + + assert len(dialog.errors) == 1 + assert dialog.gave_up == 0 # reported on its own merit, not by giving up + assert dialog._auto_timer.isActive() + + +def test_failures_inside_the_grace_window_are_not_reported(make_dialog): + """A blip (a snapshot that fails while the target loads images) stays quiet. + + Driven through the handlers rather than the timer: what matters is the + failure *sequence*, and racing a 50 ms poll to land an exact number of + failures would only make the test flaky. + """ + dialog = make_dialog() + + for _ in range(5): + dialog._handle_failed("transient") + assert dialog.errors == [] + assert dialog.gave_up == 0 + assert dialog._auto_timer.isActive() + + dialog._handle_ready(["ok"]) # healed + assert dialog._failing_since is None # the streak clock resets + + +def test_explicit_refresh_re_arms_the_poll_and_the_error_latch( + qapp, short_grace, make_dialog +): + """Reopening the dialog (the main window calls ``refresh()``) resumes it.""" + dialog = make_dialog() + dialog.failing = True + _spin(qapp, 600) + assert len(dialog.errors) == 1 + assert not dialog._auto_timer.isActive() + + dialog.failing = False + dialog.refresh() + _spin(qapp, 200) + assert dialog._auto_timer.isActive() + assert len(dialog.errors) == 1 + + # A later give-up is reported again — the latch was cleared, not sealed. + dialog.failing = True + _spin(qapp, 600) + assert len(dialog.errors) == 2 + assert dialog.gave_up == 2 + + +def test_the_close_button_tears_the_dialog_down(qapp, make_dialog): + """The Close button calls accept(), which delivers no QCloseEvent. + + The teardown used to live in ``closeEvent`` alone, so dismissing a polling + dialog the normal way left it hidden and *still polling* — unreachable + (the main window drops its reference on ``finished``), leaking a thread per + open/close cycle, and with its worker absent from the detached registry that + keeps the process handle alive while something is still reading. + """ + dialog = make_dialog() + assert dialog._auto_timer.isActive() + fetches = dialog.fetches + + dialog.accept() # exactly what the Close button is wired to + _spin(qapp, 300) + + assert not dialog.isVisible() + assert not dialog._auto_timer.isActive() + assert dialog.fetches == fetches + assert dialog._worker is None + + +def test_escape_tears_the_dialog_down_too(qapp, make_dialog): + """Esc reaches reject(), the other half of done(). + + Driven as a real key event rather than a ``reject()`` call: Esc is handled + inside ``QDialog`` on the C++ side, so this is what proves the override is + reached through Qt's virtual dispatch and not just from Python. + """ + from PySide6.QtCore import Qt + from PySide6.QtTest import QTest + + dialog = make_dialog() + dialog.show() + _spin(qapp, 50) + fetches = dialog.fetches + + QTest.keyClick(dialog, Qt.Key_Escape) + _spin(qapp, 300) + + assert not dialog.isVisible() + assert not dialog._auto_timer.isActive() + assert dialog.fetches == fetches + assert dialog._worker is None + + +def test_teardown_runs_once_however_the_dialog_is_dismissed(qapp, make_dialog): + """close() → closeEvent → reject() → done() must not tear down twice.""" + dialog = make_dialog() + calls = [] + real_teardown = dialog._teardown + dialog._teardown = lambda: (calls.append(True), real_teardown())[1] + + dialog.close() + _spin(qapp, 100) + dialog.accept() # a second dismissal must be a no-op + + assert calls == [True] diff --git a/tests/app/test_detached_worker_close.py b/tests/app/test_detached_worker_close.py new file mode 100644 index 0000000..4b568df --- /dev/null +++ b/tests/app/test_detached_worker_close.py @@ -0,0 +1,179 @@ +# -*- coding: utf-8 -*- + +""" +Tests for ``call_when_detached_workers_finish`` in ``PyMemoryEditor/app/_widgets.py``. + +``shutdown_worker_thread`` deliberately *detaches* a worker that blows its join +timeout instead of destroying it — otherwise the closing dialog would take a +running QThread down with it and abort the process. The consequence is a thread +that can still be inside a backend read after its dialog is gone, so whatever +releases the process handle it reads through (``MainWindow._change_process``) +has to wait for it rather than pull the ground out from under it. + +Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via +the ``app`` extra). +""" + +import os +import time + +import pytest + + +pytest.importorskip("PySide6", reason="App tests require PySide6 (install with [app] extra).") + +# Offscreen platform plugin: no display server needed, runs on CI. +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + + +@pytest.fixture(scope="module") +def qapp(): + """A single QApplication for the module (Qt allows only one per process).""" + from PySide6.QtWidgets import QApplication + + return QApplication.instance() or QApplication([]) + + +def _spin_until(qapp, predicate, timeout_ms=5000) -> bool: + """Pump the event loop until ``predicate`` holds (or the timeout expires).""" + from PySide6.QtCore import QElapsedTimer + + elapsed = QElapsedTimer() + elapsed.start() + while elapsed.elapsed() < timeout_ms: + if predicate(): + return True + qapp.processEvents() + time.sleep(0.005) + return predicate() + + +@pytest.fixture +def slow_worker(qapp): + """A worker that ignores the join timeout, so shutdown has to detach it.""" + from PySide6.QtCore import QThread + + class _WedgedWorker(QThread): + def __init__(self): + super().__init__() + self.busy_ms = 400 + + def run(self): + # Stands in for a backend read that can't be interrupted — the only + # reason `shutdown_worker_thread` ever detaches anything. + QThread.msleep(self.busy_ms) + + workers = [] + + def _factory(): + worker = _WedgedWorker() + workers.append(worker) + return worker + + yield _factory + + for worker in workers: + try: + worker.wait(5000) + except RuntimeError: + # Already reaped: a detached worker deleteLater()s itself once it + # finishes, which leaves the Python wrapper pointing at nothing. + pass + qapp.processEvents() + + +def test_closes_inline_when_nothing_is_detached(qapp): + """The normal path: no wedged worker, so the handle is released right away.""" + from PyMemoryEditor.app._widgets import ( + _DETACHED_WORKERS, + call_when_detached_workers_finish, + ) + + assert not [w for w in _DETACHED_WORKERS if w.isRunning()] + + closed = [] + call_when_detached_workers_finish(lambda: closed.append(True)) + assert closed == [True] # synchronous, not deferred to the event loop + + +def test_the_close_waits_for_a_detached_worker(qapp, slow_worker): + """A worker that outlived its join must not have the handle closed under it.""" + from PyMemoryEditor.app._widgets import ( + _DETACHED_WORKERS, + call_when_detached_workers_finish, + shutdown_worker_thread, + ) + + worker = slow_worker() + worker.start() + assert _spin_until(qapp, worker.isRunning, 2000) + + # A 0 ms join can't stop it, so shutdown detaches it — the situation + # _change_process has to survive. + shutdown_worker_thread(worker, wait_ms=0) + assert worker in _DETACHED_WORKERS + assert worker.isRunning() + + closed = [] + call_when_detached_workers_finish(lambda: closed.append(True)) + assert closed == [] # still reading — the handle must stay open + + assert _spin_until(qapp, lambda: bool(closed)) + assert closed == [True] + assert not worker.isRunning() + + +def test_the_blocking_wait_reports_whether_the_workers_finished(qapp, slow_worker): + """App exit has no event loop left, so it needs the blocking sibling. + + ``QThread.wait`` works without a loop; ``finished`` would never be + delivered, so ``call_when_detached_workers_finish`` can't be used there. + """ + from PyMemoryEditor.app._widgets import ( + shutdown_worker_thread, + wait_for_detached_workers, + ) + + assert wait_for_detached_workers(0) is True # nothing detached + + worker = slow_worker() + worker.busy_ms = 600 + worker.start() + assert _spin_until(qapp, worker.isRunning, 2000) + shutdown_worker_thread(worker, wait_ms=0) + + # Too short: the caller must leave the handle alone rather than close it + # under a live read. + assert wait_for_detached_workers(50) is False + assert worker.isRunning() + + # Long enough: the worker is done, so the handle is safe to release. + assert wait_for_detached_workers(3000) is True + assert not worker.isRunning() + + +def test_the_close_fires_once_for_several_detached_workers(qapp, slow_worker): + """Two wedged workers, one close — and only after the slower one is done.""" + from PyMemoryEditor.app._widgets import ( + call_when_detached_workers_finish, + shutdown_worker_thread, + ) + + quick, slow = slow_worker(), slow_worker() + quick.busy_ms, slow.busy_ms = 200, 800 + quick.start() + slow.start() + assert _spin_until(qapp, lambda: quick.isRunning() and slow.isRunning(), 2000) + + shutdown_worker_thread(quick, wait_ms=0) + shutdown_worker_thread(slow, wait_ms=0) + + closed = [] + call_when_detached_workers_finish(lambda: closed.append(True)) + + assert _spin_until(qapp, lambda: not quick.isRunning(), 3000) + qapp.processEvents() + assert closed == [] # the other one is still reading + + assert _spin_until(qapp, lambda: bool(closed)) + assert closed == [True] # exactly once, not once per worker From 1288a85c89af2f87e130e0afc655a6fd02e6a185 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 02:36:39 -0300 Subject: [PATCH 02/15] fix(app): retire only the worker that finished, and answer explicit retries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the dialog teardown work. `_on_worker_finished` cleared `self._worker` and deleted it without checking which worker had actually finished. `finished` is queued, so it can arrive after the next tick already started a replacement: a worker leaves `run()` (`isRunning()` goes False, so the in-flight guard lets the next fetch through) and its signal lands a moment later, retiring the *running* replacement instead. That drops the reference the teardown needs and schedules a live QThread for deletion, which aborts the process. Both the auto-refresh dialogs and the hex viewer (whose interval goes down to 50 ms, so the two overlap easily) now retire the worker the signal came from. An explicit `refresh()` against a still-dead target reported nothing until the grace window expired, because the window already had data from before. The flag that fixes it is bound to the fetch `refresh()` starts, so it answers that request and cannot leak into an unrelated failure later. The Threads window sat at "0 thread(s)" against a dead target on Linux and Windows, where `get_threads` reports an exited process as an empty sequence rather than raising — never reporting, never giving up, while the Memory Map and Modules windows beside it said the process was gone. A live process always has at least one thread, so an empty enumeration is now treated as the target being gone. Also: nothing may start a fetch on a dismissed dialog (the teardown latch is one-way, so such a worker could never be joined again), and the process picker joins its enumeration worker briefly instead of for a second — that join runs on the click that dismisses it, waiting on a process list nobody will read. --- PyMemoryEditor/app/_auto_refresh_dialog.py | 65 +++++++++++++--- PyMemoryEditor/app/memory_viewer_dialog.py | 19 +++-- PyMemoryEditor/app/open_process_dialog.py | 9 ++- PyMemoryEditor/app/threads_dialog.py | 13 +++- tests/app/test_auto_refresh_dialog.py | 91 ++++++++++++++++++++++ 5 files changed, 178 insertions(+), 19 deletions(-) diff --git a/PyMemoryEditor/app/_auto_refresh_dialog.py b/PyMemoryEditor/app/_auto_refresh_dialog.py index 2a2919f..e05f0a7 100644 --- a/PyMemoryEditor/app/_auto_refresh_dialog.py +++ b/PyMemoryEditor/app/_auto_refresh_dialog.py @@ -90,6 +90,8 @@ def __init__(self, process, *, refresh_interval_ms: int, parent=None): # keystroke), so the "stopped" note survives instead of being replaced # by a count that looks live. self._polling_gave_up = False + # Set by refresh() only, so it can never fire on a timer tick. + self._report_next_failure = False def _start_auto_refresh(self) -> None: """Begin polling. Call once, after the first ``refresh()``. @@ -147,10 +149,25 @@ def refresh(self) -> None: repeated failures stopped, so reopening the dialog against a live target picks the cadence back up. The timer itself goes through :meth:`_auto_refresh_tick` instead, which must *not* re-arm. + + A dismissed dialog stays dismissed: the teardown latch is one-way, so + re-arming here would leave a timer running on a dialog nothing can join + again. """ + if getattr(self, "_teardown_done", False): + return self._last_error = None self._failing_since = None self._polling_gave_up = False + # An explicit retry deserves an answer either way: without this, a + # refresh against a still-dead target would sit silent until the grace + # window expired, because `_has_data` is still True from before. + # + # ``_start_fetch`` binds it to the fetch it starts, so it answers the + # request and nothing later. A fetch already in flight when this runs + # was started before the user asked for anything, so the flag waits for + # the next one rather than being consumed by that stranger's result. + self._report_next_failure = True if self._auto_timer is not None and not self._auto_timer.isActive(): self._auto_timer.start() self._start_fetch() @@ -160,6 +177,12 @@ def _auto_refresh_tick(self) -> None: self._start_fetch() def _start_fetch(self) -> None: + # Nothing may start a fetch after the dialog was dismissed: the teardown + # latch is one-way, so a worker started here could never be joined + # again (and its dialog is on its way to being deleted). + if getattr(self, "_teardown_done", False): + return + # Skip if a fetch is already in flight — the timer would otherwise stack # workers on a slow target. This self-throttles to the real fetch time. if self._worker is not None and self._worker.isRunning(): @@ -170,10 +193,19 @@ def _start_fetch(self) -> None: if not self._has_data: self._set_loading_hint() + # Consume the "the user asked for this" flag into *this* fetch: bound to + # the outcome that answers the request, and cleared here so it can't + # leak into an unrelated failure minutes later. A refresh that found a + # fetch already in flight left it set, and it applies to this one. + asked_for_it = self._report_next_failure + self._report_next_failure = False + worker = _DataWorker(self._fetch_data, self) worker.ready.connect(self._handle_ready) - worker.failed.connect(self._handle_failed) - worker.finished.connect(self._on_worker_finished) + worker.failed.connect( + lambda message, asked=asked_for_it: self._handle_failed(message, asked) + ) + worker.finished.connect(lambda w=worker: self._on_worker_finished(w)) self._worker = worker worker.start() @@ -183,7 +215,7 @@ def _handle_ready(self, data) -> None: self._failing_since = None self._on_data_ready(data) - def _handle_failed(self, message: str) -> None: + def _handle_failed(self, message: str, asked_for_it: bool = False) -> None: """Handle a failed fetch: report it only when the user needs to know. Once the target is gone (or its handle was closed by File → Change @@ -198,7 +230,10 @@ def _handle_failed(self, message: str) -> None: * the **first** fetch fails — the window is empty, so there is nothing to look at but the error; * the poll **gives up** after ``_FAILURE_GRACE_MS`` of continuous - failure — the table is frozen on stale data from here on. + failure — the table is frozen on stale data from here on; + * the fetch was one the user explicitly asked for (``asked_for_it``, + set by :meth:`refresh`) — a retry that answers with silence is worse + than one that answers with the error. Everything in between heals quietly: the table keeps its last good rows and the next tick usually recovers. That "quietly" is load-bearing, not @@ -226,7 +261,9 @@ def _handle_failed(self, message: str) -> None: # same error quiet, and its None-ness marks a window that has never # reported anything (cleared by a successful fetch and by refresh()). never_reported_yet = not self._has_data and self._last_error is None - if (gave_up or never_reported_yet) and message != self._last_error: + if ( + gave_up or never_reported_yet or asked_for_it + ) and message != self._last_error: self._last_error = message self._on_data_failed(message) @@ -235,11 +272,19 @@ def _handle_failed(self, message: str) -> None: if gave_up: self._on_polling_stopped() - def _on_worker_finished(self) -> None: - worker = self._worker - self._worker = None - if worker is not None: - worker.deleteLater() + def _on_worker_finished(self, worker) -> None: + """Retire the worker that just finished — and only that one. + + ``finished`` is queued, so it can arrive *after* the next tick already + started a replacement: a worker exits ``run()`` (``isRunning()`` goes + False, the guard in :meth:`_start_fetch` lets the next fetch through) + and its signal is delivered a moment later. Clearing ``self._worker`` + blindly there would drop the reference to the *running* replacement and + ``deleteLater()`` it — destroying a live QThread aborts the process. + """ + if worker is self._worker: + self._worker = None + worker.deleteLater() def _teardown(self) -> None: if self._auto_timer is not None: diff --git a/PyMemoryEditor/app/memory_viewer_dialog.py b/PyMemoryEditor/app/memory_viewer_dialog.py index 83e5be9..65e8509 100644 --- a/PyMemoryEditor/app/memory_viewer_dialog.py +++ b/PyMemoryEditor/app/memory_viewer_dialog.py @@ -195,7 +195,7 @@ def fetch(): worker = _DataWorker(fetch, self) worker.ready.connect(self._on_read_result) - worker.finished.connect(self._on_worker_finished) + worker.finished.connect(lambda w=worker: self._on_worker_finished(w)) self._worker = worker worker.start() @@ -217,11 +217,18 @@ def _on_read_result(self, result) -> None: self._dump.setPlainText(_format_hex_dump(addr, data)) self._status.setText(f"Read {len(data):,} bytes from 0x{addr:X}") - def _on_worker_finished(self) -> None: - worker = self._worker - self._worker = None - if worker is not None: - worker.deleteLater() + def _on_worker_finished(self, worker) -> None: + """Retire the worker that just finished — and only that one. + + ``finished`` is queued and can land after the next auto-refresh tick + started a replacement (the interval goes down to 50 ms here, so the two + overlap easily). Clearing ``self._worker`` blindly would drop the + reference to the running replacement and ``deleteLater()`` it — + destroying a live QThread aborts the process. + """ + if worker is self._worker: + self._worker = None + worker.deleteLater() def _toggle_auto(self, on: bool) -> None: self._auto_btn.setText("Auto-refresh: On" if on else "Auto-refresh: Off") diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index 0dbb9c7..f8afb79 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -330,10 +330,15 @@ def _on_scan_finished(self) -> None: def _teardown(self) -> None: self._refresh_timer.stop() # Hand the enumeration worker to the shared shutdown: it disconnects, - # joins, and — when psutil is wedged on a huge process list — detaches + # joins, and — when psutil is slow on a huge process list — detaches # instead of leaving a live QThread parented to a dialog that is about # to be dropped (destroying one aborts the process). - shutdown_worker_thread(self._scan_worker, wait_ms=1000) + # + # The join is deliberately short. This runs on the click that dismisses + # the picker, and its result is already irrelevant: whoever waits is + # waiting for a process list nobody will read. Long enough to absorb a + # scan that is nearly done, short enough to be imperceptible. + shutdown_worker_thread(self._scan_worker, wait_ms=250) self._scan_worker = None def _on_filter_changed(self, text: str) -> None: diff --git a/PyMemoryEditor/app/threads_dialog.py b/PyMemoryEditor/app/threads_dialog.py index 495ff77..c542d27 100644 --- a/PyMemoryEditor/app/threads_dialog.py +++ b/PyMemoryEditor/app/threads_dialog.py @@ -106,7 +106,18 @@ def _set_loading_hint(self) -> None: self._count_label.setText("Enumerating threads…") def _fetch_data(self): - return list(self._process.get_threads()) + threads = list(self._process.get_threads()) + # A live process always has at least one thread, and `get_threads` does + # not report an exited one uniformly: Linux walks /proc//task and + # Windows filters a system-wide snapshot, so both simply come up empty + # where macOS raises. Without this the window would sit at "0 thread(s)" + # against a dead target, never reporting and never giving up, while the + # Memory Map and Modules windows next to it say the process is gone. + if not threads: + raise RuntimeError( + "The process reported no threads — it has most likely exited." + ) + return threads def _on_data_ready(self, threads) -> None: self._threads = list(threads) diff --git a/tests/app/test_auto_refresh_dialog.py b/tests/app/test_auto_refresh_dialog.py index 3597e26..86f555f 100644 --- a/tests/app/test_auto_refresh_dialog.py +++ b/tests/app/test_auto_refresh_dialog.py @@ -275,3 +275,94 @@ def test_teardown_runs_once_however_the_dialog_is_dismissed(qapp, make_dialog): dialog.accept() # a second dismissal must be a no-op assert calls == [True] + + +def test_a_late_finished_does_not_retire_the_worker_that_replaced_it(make_dialog): + """``finished`` is queued and can land after the next tick started a new fetch. + + Retiring ``self._worker`` blindly there dropped the reference to the + *running* replacement and scheduled it for deletion — destroying a live + QThread aborts the process, and the teardown could no longer join it. + """ + dialog = make_dialog() + dialog._start_fetch() + current = dialog._worker + assert current is not None + + class _Stale: + def __init__(self): + self.deleted = False + + def deleteLater(self): + self.deleted = True + + stale = _Stale() + dialog._on_worker_finished(stale) + + assert dialog._worker is current # still tracked, still joinable + assert stale.deleted # and the one that really finished is retired + + +def test_an_explicit_refresh_reports_its_failure_immediately(qapp, make_dialog): + """A retry the user asked for answers now, not after the grace window.""" + dialog = make_dialog() # production grace: 3 s + dialog.failing = True + + dialog.refresh() + _spin(qapp, 200) + + assert len(dialog.errors) == 1 + assert dialog.gave_up == 0 # reported on its own merit, not by giving up + assert dialog._auto_timer.isActive() + + +def test_nothing_fetches_after_the_dialog_was_torn_down(qapp, make_dialog): + """The teardown latch is one-way, so a fetch started later is unjoinable.""" + dialog = make_dialog() + dialog.accept() + _spin(qapp, 100) + fetches = dialog.fetches + + dialog.refresh() # a stale caller poking a dismissed dialog + _spin(qapp, 200) + + assert dialog.fetches == fetches + assert dialog._worker is None + assert not dialog._auto_timer.isActive() + + +def test_threads_dialog_treats_an_empty_enumeration_as_a_dead_target(qapp, short_grace): + """Linux and Windows report an exited process as *no threads*, not an error. + + Without this the window would sit at "0 thread(s)" against a dead target — + never reporting, never giving up — while the Memory Map and Modules windows + beside it say the process is gone. + """ + from PyMemoryEditor.app.threads_dialog import ThreadsDialog + from PyMemoryEditor.process.thread_info import ThreadInfo + + alive = [ThreadInfo(tid=1, start_address=None, state="S", priority=20, raw="1")] + state = {"threads": alive} + + class _Process: + pid = 4242 + + def get_threads(self): + return iter(state["threads"]) + + reported = [] + dialog = ThreadsDialog(_Process()) + dialog._on_data_failed = lambda message: reported.append(message) + try: + _spin(qapp, 400) + assert dialog._model.rowCount() == 1 + assert reported == [] + + state["threads"] = [] # the target exits + _spin(qapp, 600) + + assert len(reported) == 1 + assert "exited" in reported[0] + finally: + dialog.close() + _spin(qapp, 50) From f8f7c0defbf716e5f89054632bd380e95aea5397 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 02:42:54 -0300 Subject: [PATCH 03/15] fix(app): apply the stale-`finished` guard to the two remaining slots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The process picker and the pointer scan kept the shape that was fixed in the polling dialogs: retire whatever `self._worker` currently points at, regardless of which worker the queued `finished` came from. The picker is genuinely exposed — it re-enumerates every 3 s, and a worker leaves `run()` before its signal is delivered, so the in-flight guard can let a replacement start in that window and the late signal then deletes the running one. The pointer scan can't hit it today (the Scan button stays disabled until this very slot re-enables it, so no scan can be replaced mid-flight), but it now matches: re-enabling the UI or dropping the worker on behalf of a superseded scan would be wrong the moment that precondition changes. --- PyMemoryEditor/app/open_process_dialog.py | 20 ++++++++++++++------ PyMemoryEditor/app/pointer_scan_dialog.py | 20 +++++++++++++++----- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index f8afb79..2d70902 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -289,7 +289,7 @@ def _populate_processes(self) -> None: worker = _ProcessListWorker(self) worker.rows_ready.connect(self._on_rows_ready) - worker.finished.connect(self._on_scan_finished) + worker.finished.connect(lambda w=worker: self._on_scan_finished(w)) self._scan_worker = worker worker.start() @@ -321,11 +321,19 @@ def _on_rows_ready(self, rows) -> None: self._table.selectRow(row) break - def _on_scan_finished(self) -> None: - worker = self._scan_worker - self._scan_worker = None - if worker is not None: - worker.deleteLater() + def _on_scan_finished(self, worker) -> None: + """Retire the enumeration that just finished — and only that one. + + ``finished`` is queued, so it can land after a refresh already started a + replacement (the worker leaves ``run()`` before its signal is + delivered, and the in-flight guard in ``_populate_processes`` lets the + next scan through in that window). Clearing ``_scan_worker`` blindly + would drop the reference to the running replacement and + ``deleteLater()`` it, which aborts the process. + """ + if worker is self._scan_worker: + self._scan_worker = None + worker.deleteLater() def _teardown(self) -> None: self._refresh_timer.stop() diff --git a/PyMemoryEditor/app/pointer_scan_dialog.py b/PyMemoryEditor/app/pointer_scan_dialog.py index d16e4bb..cc60897 100644 --- a/PyMemoryEditor/app/pointer_scan_dialog.py +++ b/PyMemoryEditor/app/pointer_scan_dialog.py @@ -626,7 +626,7 @@ def _start_worker(self, worker: QThread) -> None: worker.rows_ready.connect(self._append_rows) worker.error.connect(self._on_error) worker.finished_ok.connect(self._on_finished_ok) - worker.finished.connect(self._on_worker_finished) + worker.finished.connect(lambda w=worker: self._on_worker_finished(w)) self._worker = worker worker.start() @@ -736,12 +736,22 @@ def _on_finished_ok(self, count: int) -> None: self._count_label.setText(f"{shown:,} pointer path(s).") self._export_btn.setEnabled(shown > 0) - def _on_worker_finished(self) -> None: - worker = self._worker + def _on_worker_finished(self, worker) -> None: + """Retire the scan that just finished — and only that one. + + No scan can currently be replaced while another is running (the Scan + button is disabled until this slot re-enables it), so the queued-signal + race that bites the polling dialogs can't happen here yet. Matching + their shape anyway: re-enabling the UI or dropping ``_worker`` on behalf + of a scan that was already superseded would be wrong the moment that + precondition changes. + """ + if worker is not self._worker: + worker.deleteLater() + return self._worker = None self._set_scanning(False) - if worker is not None: - worker.deleteLater() + worker.deleteLater() # ----------------------------------------------------------- actions --- # From e53527115d8b8745c801919164e7416e33c7c40c Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 10:48:16 -0300 Subject: [PATCH 04/15] test(app): stop the dialog tests from leaving the process crashy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI hit two failures that were both in the new tests, not in the app. `tests/app/test_auto_refresh_dialog.py` left the Qt process in a state where a *later* test segfaulted inside pytest-qt's `qtbot.wait` — enough to take an xdist worker down with "node down: Not properly terminated", after which the run hung until the 6 h job timeout. Reproducible deterministically with pytest tests/app/test_auto_refresh_dialog.py tests/app/test_app_smoke.py The dialogs were parentless, so each one was owned by its Python wrapper and destroyed the instant the test's reference went away, taking its worker children with it while the event queue could still hold events for them. They now hang off a module-scoped parent widget, so Qt owns them and nothing is freed underneath a queued event. They are still closed per test — that is what exercises the teardown. `test_the_close_waits_for_a_detached_worker` also failed on Windows with "Internal C++ object (_WedgedWorker) already deleted": once the worker finishes, the detach machinery reaps it, so asserting on `isRunning()` afterwards touches a deleted object. The worker now sets a plain Python flag when its `run()` completes, and the assertions use that. --- tests/app/test_auto_refresh_dialog.py | 38 +++++++++++++++++++------ tests/app/test_detached_worker_close.py | 20 +++++++++---- 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/tests/app/test_auto_refresh_dialog.py b/tests/app/test_auto_refresh_dialog.py index 86f555f..bc636bd 100644 --- a/tests/app/test_auto_refresh_dialog.py +++ b/tests/app/test_auto_refresh_dialog.py @@ -60,20 +60,40 @@ def short_grace(monkeypatch): return 150 +@pytest.fixture(scope="module") +def dialog_parent(qapp): + """A hidden parent, so Qt owns the dialogs instead of Python's GC. + + A parentless QDialog is owned by its Python wrapper: it is destroyed the + instant the test's reference goes away, taking its worker children with it + while the event queue may still hold events for them. That left the process + crashing later — a segfault inside a *different* test's ``qtbot.wait``. + Parenting them here keeps every object alive until the module is done, so + nothing is freed underneath a queued event. + + Deliberately never deleted: the process is about to exit anyway, and a + teardown here would recreate the very window this avoids. + """ + from PySide6.QtWidgets import QWidget + + return QWidget() + + @pytest.fixture -def make_dialog(qapp): +def make_dialog(qapp, dialog_parent): """Factory for an ``AutoRefreshTableDialog`` whose fetch can be made to fail. - The dialogs are parentless here, so PySide6 destroys the C++ object as soon - as the Python reference goes away. The fixture closes them (joining the - fetch worker) and pumps the loop once more, so no live QThread is left - hanging off an object about to be deleted. + Dialogs are closed at the end of each test — that is what exercises the + teardown — but stay owned by ``dialog_parent`` (see there) rather than being + destroyed mid-session. """ from PyMemoryEditor.app._auto_refresh_dialog import AutoRefreshTableDialog class _Dialog(AutoRefreshTableDialog): def __init__(self, interval_ms, failing): - super().__init__(object(), refresh_interval_ms=interval_ms) + super().__init__( + object(), refresh_interval_ms=interval_ms, parent=dialog_parent + ) self.failing = failing self.flapping = False self.fetches = 0 @@ -331,7 +351,9 @@ def test_nothing_fetches_after_the_dialog_was_torn_down(qapp, make_dialog): assert not dialog._auto_timer.isActive() -def test_threads_dialog_treats_an_empty_enumeration_as_a_dead_target(qapp, short_grace): +def test_threads_dialog_treats_an_empty_enumeration_as_a_dead_target( + qapp, short_grace, dialog_parent +): """Linux and Windows report an exited process as *no threads*, not an error. Without this the window would sit at "0 thread(s)" against a dead target — @@ -351,7 +373,7 @@ def get_threads(self): return iter(state["threads"]) reported = [] - dialog = ThreadsDialog(_Process()) + dialog = ThreadsDialog(_Process(), parent=dialog_parent) dialog._on_data_failed = lambda message: reported.append(message) try: _spin(qapp, 400) diff --git a/tests/app/test_detached_worker_close.py b/tests/app/test_detached_worker_close.py index 4b568df..ae77e09 100644 --- a/tests/app/test_detached_worker_close.py +++ b/tests/app/test_detached_worker_close.py @@ -57,11 +57,17 @@ class _WedgedWorker(QThread): def __init__(self): super().__init__() self.busy_ms = 400 + # Python-side completion flag. Assertions must not call isRunning() + # after the worker finishes: the detach machinery reaps it (a + # deleteLater on the C++ object), and touching the wrapper then + # raises "Internal C++ object already deleted". + self.ran_to_completion = False def run(self): # Stands in for a backend read that can't be interrupted — the only # reason `shutdown_worker_thread` ever detaches anything. QThread.msleep(self.busy_ms) + self.ran_to_completion = True workers = [] @@ -85,11 +91,15 @@ def _factory(): def test_closes_inline_when_nothing_is_detached(qapp): """The normal path: no wedged worker, so the handle is released right away.""" from PyMemoryEditor.app._widgets import ( - _DETACHED_WORKERS, call_when_detached_workers_finish, + wait_for_detached_workers, ) - assert not [w for w in _DETACHED_WORKERS if w.isRunning()] + # Precondition: nothing wedged from an earlier test. A finished worker may + # still sit in the registry waiting for its reaper, which doesn't count — + # and touching such a wrapper directly can hit an already-deleted C++ + # object, so ask through the guarded helper. + assert wait_for_detached_workers(0) is True closed = [] call_when_detached_workers_finish(lambda: closed.append(True)) @@ -120,7 +130,7 @@ def test_the_close_waits_for_a_detached_worker(qapp, slow_worker): assert _spin_until(qapp, lambda: bool(closed)) assert closed == [True] - assert not worker.isRunning() + assert worker.ran_to_completion def test_the_blocking_wait_reports_whether_the_workers_finished(qapp, slow_worker): @@ -149,7 +159,7 @@ def test_the_blocking_wait_reports_whether_the_workers_finished(qapp, slow_worke # Long enough: the worker is done, so the handle is safe to release. assert wait_for_detached_workers(3000) is True - assert not worker.isRunning() + assert worker.ran_to_completion def test_the_close_fires_once_for_several_detached_workers(qapp, slow_worker): @@ -171,7 +181,7 @@ def test_the_close_fires_once_for_several_detached_workers(qapp, slow_worker): closed = [] call_when_detached_workers_finish(lambda: closed.append(True)) - assert _spin_until(qapp, lambda: not quick.isRunning(), 3000) + assert _spin_until(qapp, lambda: quick.ran_to_completion, 3000) qapp.processEvents() assert closed == [] # the other one is still reading From d6c076558facf8d4ebb6bf9e03ea775a9fd8b199 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 14:42:39 -0300 Subject: [PATCH 05/15] fix(app): give the hex viewer the same give-up as the other pollers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The Hex Viewer is the one polling window that never gave up: against a dead target its read fails on every tick and each one logged a warning — up to 20 a second at the 50 ms interval floor — so the Log Console filled with the same line forever. That is the spam of issue #74 wearing a different hat, reported through the logger instead of a modal. It now logs the first tick of a failure streak rather than every one, and stops polling after the same grace window the other dialogs use. Stopping goes through the auto-refresh button, so the UI can't claim to be refreshing while it isn't, and re-enabling it hands the target a fresh grace window. Also guards the pending-worker snapshot in `call_when_detached_workers_finish` against a worker reaped between the listing and the check, matching `wait_for_detached_workers` and the loop just below it — the same list with the same hazard, and an escape there would abort a process switch mid-teardown. --- PyMemoryEditor/app/_widgets.py | 13 +++++- PyMemoryEditor/app/memory_viewer_dialog.py | 54 ++++++++++++++++++---- tests/app/test_auto_refresh_dialog.py | 41 ++++++++++++++++ 3 files changed, 98 insertions(+), 10 deletions(-) diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index fae1316..c7de2ca 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -133,7 +133,18 @@ def call_when_detached_workers_finish(callback: Callable[[], None]) -> None: freeing memory the app needs back). Callers that cannot wait for an event loop want :func:`wait_for_detached_workers`, which is bounded instead. """ - pending = [worker for worker in _DETACHED_WORKERS if worker.isRunning()] + pending = [] + for worker in list(_DETACHED_WORKERS): + try: + if worker.isRunning(): + pending.append(worker) + except RuntimeError: + # Reaped and deleted between the snapshot and here — it can't be + # reading anything any more. Same tolerance as the loop below and + # as wait_for_detached_workers; letting it escape would abort a + # process switch mid-teardown. + continue + if not pending: callback() return diff --git a/PyMemoryEditor/app/memory_viewer_dialog.py b/PyMemoryEditor/app/memory_viewer_dialog.py index 65e8509..cea330a 100644 --- a/PyMemoryEditor/app/memory_viewer_dialog.py +++ b/PyMemoryEditor/app/memory_viewer_dialog.py @@ -8,7 +8,7 @@ import logging from typing import Optional -from PySide6.QtCore import QTimer +from PySide6.QtCore import QElapsedTimer, QTimer from PySide6.QtGui import QFont from PySide6.QtWidgets import ( QDialog, @@ -24,7 +24,7 @@ from PyMemoryEditor import AbstractProcess -from ._auto_refresh_dialog import _DataWorker +from ._auto_refresh_dialog import _FAILURE_GRACE_MS, _DataWorker from ._widgets import TearsDownOnClose, parse_hex_address, shutdown_worker_thread @@ -146,6 +146,15 @@ def _build_ui(self) -> None: self._timer = QTimer(self) self._timer.timeout.connect(self.refresh) + # Failure bookkeeping, same shape as the auto-refresh dialogs: report a + # given error once rather than once per tick, and stop polling a target + # that keeps failing. Without it a dead target logged a warning on every + # tick forever — up to 20 a second at the 50 ms floor — which is the + # spam of issue #74 wearing a different hat (the Log Console instead of + # a modal). + self._last_error: Optional[str] = None + self._failing_since: Optional[QElapsedTimer] = None + def _parse_address(self) -> Optional[int]: text = self._addr_edit.text().strip() if not text: @@ -204,16 +213,40 @@ def _on_read_result(self, result) -> None: addr, size, data, exc = result if exc is not None: self._dump.setPlainText("") - self._status.setText(f"Read failed: {type(exc).__name__}: {exc}") - _LOG.warning( - "Hex viewer read failed at 0x%X (%d bytes): %s: %s", - addr, - size, - type(exc).__name__, - exc, + message = f"{type(exc).__name__}: {exc}" + + # Log the first tick of a streak, not every one of them. + if message != self._last_error: + self._last_error = message + _LOG.warning( + "Hex viewer read failed at 0x%X (%d bytes): %s", + addr, + size, + message, + ) + + gave_up = False + if self._failing_since is None: + self._failing_since = QElapsedTimer() + self._failing_since.start() + elif ( + self._failing_since.elapsed() >= _FAILURE_GRACE_MS + and self._auto_btn.isChecked() + ): + # Turning the button off stops the timer through the normal + # slot, so the UI can't claim to be refreshing while it isn't — + # and the user re-enables it with the same button. + gave_up = True + self._auto_btn.setChecked(False) + + self._status.setText( + f"Read failed: {message}" + + (" — auto-refresh stopped." if gave_up else "") ) return + self._last_error = None + self._failing_since = None self._dump.setPlainText(_format_hex_dump(addr, data)) self._status.setText(f"Read {len(data):,} bytes from 0x{addr:X}") @@ -233,6 +266,9 @@ def _on_worker_finished(self, worker) -> None: def _toggle_auto(self, on: bool) -> None: self._auto_btn.setText("Auto-refresh: On" if on else "Auto-refresh: Off") if on: + # Turning it back on is the user saying "try again": give the target + # a fresh grace window instead of one already spent. + self._failing_since = None self._sync_timer() else: self._timer.stop() diff --git a/tests/app/test_auto_refresh_dialog.py b/tests/app/test_auto_refresh_dialog.py index bc636bd..28eb12a 100644 --- a/tests/app/test_auto_refresh_dialog.py +++ b/tests/app/test_auto_refresh_dialog.py @@ -388,3 +388,44 @@ def get_threads(self): finally: dialog.close() _spin(qapp, 50) + + +def test_hex_viewer_stops_polling_and_logs_once_on_a_dead_target( + qapp, dialog_parent, caplog, monkeypatch +): + """The Hex Viewer is the other polling window, and it never gave up. + + Its reads fail every tick against a dead target, and each one logged a + warning — up to 20 a second at the 50 ms floor, straight into the Log + Console. Same defect as issue #74, reported through the logger instead of a + modal. + """ + import logging + + from PyMemoryEditor.app import memory_viewer_dialog as mv + + monkeypatch.setattr(mv, "_FAILURE_GRACE_MS", 150) + + class _Process: + pid = 4242 + + def read_process_memory(self, address, pytype, length): + raise OSError("the process is gone") + + viewer = mv.MemoryViewerDialog( + _Process(), address=0x1000, length=64, parent=dialog_parent + ) + try: + viewer._interval_spin.setValue(50) + with caplog.at_level(logging.WARNING, logger="PyMemoryEditor"): + viewer._auto_btn.setChecked(True) # the user opts into polling + _spin(qapp, 600) + + warnings = [r for r in caplog.records if "Hex viewer read failed" in r.message] + assert len(warnings) == 1 # once per streak, not once per tick + assert not viewer._timer.isActive() + assert not viewer._auto_btn.isChecked() # and the button says so + assert "auto-refresh stopped" in viewer._status.text() + finally: + viewer.close() + _spin(qapp, 50) From 6a2792f72e63439ac5f96351c190d6afd20e1f78 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 14:49:30 -0300 Subject: [PATCH 06/15] fix(app): key the hex viewer's read-failure log by range, not just message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the previous commit. Deduping the log line by message alone went quiet about a *different* range that started failing after the user moved the viewer — the point is to stop repeating the same line every tick, not to hide a new problem. The key is now (address, size, message). Also notes in `_FAILURE_GRACE_MS` that the Hex Viewer shares the budget (its interval goes down to 50 ms, so a tick count would have been an order of magnitude off), and documents the viewer's behaviour alongside the other windows in docs/app.md. --- PyMemoryEditor/app/_auto_refresh_dialog.py | 7 ++++--- PyMemoryEditor/app/memory_viewer_dialog.py | 11 +++++++---- docs/app.md | 4 ++++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/PyMemoryEditor/app/_auto_refresh_dialog.py b/PyMemoryEditor/app/_auto_refresh_dialog.py index e05f0a7..f2e696d 100644 --- a/PyMemoryEditor/app/_auto_refresh_dialog.py +++ b/PyMemoryEditor/app/_auto_refresh_dialog.py @@ -32,9 +32,10 @@ # would freeze a window that would have recovered on the next tick. # # Measured in wall-clock rather than ticks on purpose: "how long does the target -# get to recover" is the same question for every dialog, but the intervals are -# not (300 ms for Threads, 1000 ms for Memory Map / Modules), so a tick count -# would hand them budgets that differ by 3x. What gets *reported* is decided +# get to recover" is the same question for every window, but the intervals are +# not (300 ms for Threads, 1000 ms for Memory Map / Modules, and anything from +# 50 ms up in the Hex Viewer, which shares this budget), so a tick count would +# hand them budgets differing by more than an order of magnitude. What gets *reported* is decided # separately, in ``_handle_failed`` — this constant only bounds how long a # failing target keeps a worker running. _FAILURE_GRACE_MS = 3000 diff --git a/PyMemoryEditor/app/memory_viewer_dialog.py b/PyMemoryEditor/app/memory_viewer_dialog.py index cea330a..ac3d973 100644 --- a/PyMemoryEditor/app/memory_viewer_dialog.py +++ b/PyMemoryEditor/app/memory_viewer_dialog.py @@ -152,7 +152,10 @@ def _build_ui(self) -> None: # tick forever — up to 20 a second at the 50 ms floor — which is the # spam of issue #74 wearing a different hat (the Log Console instead of # a modal). - self._last_error: Optional[str] = None + # Keyed by (address, size, message): the point is to stop repeating the + # same line every tick, not to go quiet about a *different* range that + # started failing after the user moved the viewer. + self._last_failure: Optional[tuple] = None self._failing_since: Optional[QElapsedTimer] = None def _parse_address(self) -> Optional[int]: @@ -216,8 +219,8 @@ def _on_read_result(self, result) -> None: message = f"{type(exc).__name__}: {exc}" # Log the first tick of a streak, not every one of them. - if message != self._last_error: - self._last_error = message + if (addr, size, message) != self._last_failure: + self._last_failure = (addr, size, message) _LOG.warning( "Hex viewer read failed at 0x%X (%d bytes): %s", addr, @@ -245,7 +248,7 @@ def _on_read_result(self, result) -> None: ) return - self._last_error = None + self._last_failure = None self._failing_since = None self._dump.setPlainText(_format_hex_dump(addr, data)) self._status.setText(f"Read {len(data):,} bytes from 0x{addr:X}") diff --git a/docs/app.md b/docs/app.md index 2f4fb72..6bf8bbb 100644 --- a/docs/app.md +++ b/docs/app.md @@ -101,6 +101,10 @@ reports the failure once, stops refreshing, and says so in its status line still copy an address out of it. Close and reopen the window to point it at a live target again. +The Hex Viewer's optional auto-refresh follows the same rule: it reports the +failed read once, switches itself off, and says so — rather than retrying (and +logging) several times a second against a target that is gone. + Brief hiccups don't trigger any of that — a read that fails and recovers within a few seconds is left alone, so a busy target loading libraries won't interrupt you. From 48cf977193afd0c7eeab511331d465aaa8b8ea5f Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 15:14:55 -0300 Subject: [PATCH 07/15] docs(app): drop the 'When the target exits' section --- docs/app.md | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/docs/app.md b/docs/app.md index 6bf8bbb..46a68c6 100644 --- a/docs/app.md +++ b/docs/app.md @@ -87,28 +87,6 @@ from the **Theme** button on the toolbar; your choice is remembered between runs 6. (Optional) **Run a Pointer Scan** on the result to find a chain that survives restarts. -## When the target exits - -The app polls the target while you work — the Memory Map, Modules and Threads -windows re-read it on a timer (a few times a second for Threads), and the main -window checks every couple of seconds that it is still alive. - -When the process goes away (or you switch to another one via -`File → Change Process…`), you get **one** notice per window rather than one per -poll: the main window tells you the target exited, and any open auxiliary window -reports the failure once, stops refreshing, and says so in its status line -(`auto-refresh stopped`). Its table stays on the last data it read, so you can -still copy an address out of it. Close and reopen the window to point it at a -live target again. - -The Hex Viewer's optional auto-refresh follows the same rule: it reports the -failed read once, switches itself off, and says so — rather than retrying (and -logging) several times a second against a target that is gone. - -Brief hiccups don't trigger any of that — a read that fails and recovers within -a few seconds is left alone, so a busy target loading libraries won't interrupt -you. - ## Importing & exporting The Cheat Table and Pointer Scan results are stored as plain **JSON**, so you From d4d1fa4af9786c800c949d7d07244a5cde68f1aa Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 15:25:53 -0300 Subject: [PATCH 08/15] refactor(app): drop hardening this issue didn't need MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scope trim after re-reading the whole change. Three pieces went in during review rounds rather than from evidence about issue #74, and each cost more than it bought. * The deferred process-handle release. Two helpers, an app-exit hook and a test module existed to keep a handle open until every detached worker had finished. What they protected against is a read whose result is already discarded: those fetches are read-only and their signals are disconnected before the handle goes. `_change_process` closes inline again, with the residual window written down where it happens. The cheat poller still goes through `shutdown_worker_thread`, which is not cosmetic — deleting a widget that owns a running QThread aborts the process. * `_report_next_failure`. It answered an explicit refresh right away instead of at the give-up, which only differs when the user re-opens an already-open window from the menu. A fourth piece of state in a small state machine, for a case where the status line already shows the previous failure. * The stale-`finished` guard in the pointer scan, which the scan button's own disabled state already makes unreachable. `application.main` keeps closing `window.process` rather than the handle the picker opened — that one was a real bug, not hardening. --- PyMemoryEditor/app/_auto_refresh_dialog.py | 36 +--- PyMemoryEditor/app/_widgets.py | 116 +------------ PyMemoryEditor/app/application.py | 21 +-- PyMemoryEditor/app/cheat_table.py | 5 +- PyMemoryEditor/app/main_window.py | 28 ++- PyMemoryEditor/app/pointer_scan_dialog.py | 20 +-- tests/app/test_auto_refresh_dialog.py | 13 -- tests/app/test_detached_worker_close.py | 189 --------------------- 8 files changed, 36 insertions(+), 392 deletions(-) delete mode 100644 tests/app/test_detached_worker_close.py diff --git a/PyMemoryEditor/app/_auto_refresh_dialog.py b/PyMemoryEditor/app/_auto_refresh_dialog.py index f2e696d..1097314 100644 --- a/PyMemoryEditor/app/_auto_refresh_dialog.py +++ b/PyMemoryEditor/app/_auto_refresh_dialog.py @@ -91,8 +91,6 @@ def __init__(self, process, *, refresh_interval_ms: int, parent=None): # keystroke), so the "stopped" note survives instead of being replaced # by a count that looks live. self._polling_gave_up = False - # Set by refresh() only, so it can never fire on a timer tick. - self._report_next_failure = False def _start_auto_refresh(self) -> None: """Begin polling. Call once, after the first ``refresh()``. @@ -124,6 +122,9 @@ def _on_data_failed(self, message: str) -> None: Called only for a failure the user needs to see — the first fetch, or the one that makes the poll give up — and never twice for the same error (see :meth:`_handle_failed`), so a modal dialog is safe here. + A retry the user asks for while the window already has data waits for + that give-up rather than reporting on the spot; the status line still + carries the previous failure in the meantime. """ raise NotImplementedError @@ -160,15 +161,6 @@ def refresh(self) -> None: self._last_error = None self._failing_since = None self._polling_gave_up = False - # An explicit retry deserves an answer either way: without this, a - # refresh against a still-dead target would sit silent until the grace - # window expired, because `_has_data` is still True from before. - # - # ``_start_fetch`` binds it to the fetch it starts, so it answers the - # request and nothing later. A fetch already in flight when this runs - # was started before the user asked for anything, so the flag waits for - # the next one rather than being consumed by that stranger's result. - self._report_next_failure = True if self._auto_timer is not None and not self._auto_timer.isActive(): self._auto_timer.start() self._start_fetch() @@ -194,18 +186,9 @@ def _start_fetch(self) -> None: if not self._has_data: self._set_loading_hint() - # Consume the "the user asked for this" flag into *this* fetch: bound to - # the outcome that answers the request, and cleared here so it can't - # leak into an unrelated failure minutes later. A refresh that found a - # fetch already in flight left it set, and it applies to this one. - asked_for_it = self._report_next_failure - self._report_next_failure = False - worker = _DataWorker(self._fetch_data, self) worker.ready.connect(self._handle_ready) - worker.failed.connect( - lambda message, asked=asked_for_it: self._handle_failed(message, asked) - ) + worker.failed.connect(self._handle_failed) worker.finished.connect(lambda w=worker: self._on_worker_finished(w)) self._worker = worker worker.start() @@ -216,7 +199,7 @@ def _handle_ready(self, data) -> None: self._failing_since = None self._on_data_ready(data) - def _handle_failed(self, message: str, asked_for_it: bool = False) -> None: + def _handle_failed(self, message: str) -> None: """Handle a failed fetch: report it only when the user needs to know. Once the target is gone (or its handle was closed by File → Change @@ -231,10 +214,7 @@ def _handle_failed(self, message: str, asked_for_it: bool = False) -> None: * the **first** fetch fails — the window is empty, so there is nothing to look at but the error; * the poll **gives up** after ``_FAILURE_GRACE_MS`` of continuous - failure — the table is frozen on stale data from here on; - * the fetch was one the user explicitly asked for (``asked_for_it``, - set by :meth:`refresh`) — a retry that answers with silence is worse - than one that answers with the error. + failure — the table is frozen on stale data from here on. Everything in between heals quietly: the table keeps its last good rows and the next tick usually recovers. That "quietly" is load-bearing, not @@ -262,9 +242,7 @@ def _handle_failed(self, message: str, asked_for_it: bool = False) -> None: # same error quiet, and its None-ness marks a window that has never # reported anything (cleared by a successful fetch and by refresh()). never_reported_yet = not self._has_data and self._last_error is None - if ( - gave_up or never_reported_yet or asked_for_it - ) and message != self._last_error: + if (gave_up or never_reported_yet) and message != self._last_error: self._last_error = message self._on_data_failed(message) diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index c7de2ca..1f0e104 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -8,7 +8,7 @@ from typing import Any, Callable, Iterable, List, Optional, Tuple -from PySide6.QtCore import QElapsedTimer, Qt, QThread +from PySide6.QtCore import Qt, QThread from PySide6.QtGui import QStandardItem @@ -63,10 +63,8 @@ def detach_worker(worker: QThread) -> None: Reparents it away from the dying widget and holds it in the module-level registry until it finishes on its own. Callers that stop workers their own way (``MainWindow._shutdown_worker`` pumps the event loop for its blocking - connections) must park them *here* rather than in a list of their own: - ``wait_for_detached_workers`` and ``call_when_detached_workers_finish`` - protect a process handle from exactly these threads, and a second registry - is a set of threads they cannot see. + connections) park them here too, so there is one place to look for threads + that outlived their owner rather than one list per caller. """ worker.setParent(None) _DETACHED_WORKERS.append(worker) @@ -79,109 +77,6 @@ def _reap_detached_worker(worker: QThread) -> None: worker.deleteLater() -def wait_for_detached_workers(timeout_ms: int = 2000) -> bool: - """Block until no detached worker is running. Returns whether that happened. - - The blocking sibling of :func:`call_when_detached_workers_finish`, for - teardown that has no event loop left to deliver ``finished`` — application - exit, once ``app.exec()`` has returned. ``QThread.wait`` needs no loop, so - this is the only shape that works there. - - ``False`` means a worker is still wedged in a backend read after - ``timeout_ms``; the caller must then leave whatever that worker reads - through alone (at exit, the OS reclaims it moments later anyway). - """ - deadline_left = timeout_ms - for worker in list(_DETACHED_WORKERS): - try: - if not worker.isRunning(): - continue - elapsed = QElapsedTimer() - elapsed.start() - worker.wait(max(deadline_left, 0)) - deadline_left -= int(elapsed.elapsed()) - if worker.isRunning(): - return False - except RuntimeError: - # Already reaped and deleted between the snapshot and here. - continue - return True - - -def call_when_detached_workers_finish(callback: Callable[[], None]) -> None: - """Run ``callback`` once no currently-detached worker can still be running. - - Detaching (see :func:`shutdown_worker_thread`) keeps a wedged thread alive - *past* the dialog that owned it — which is the whole point, but it means the - thread may still be inside a backend read after its dialog is gone. Anything - that pulls the ground out from under such a read (closing the process handle - the worker reads through) has to wait for it, and this is how it waits - without blocking the UI thread. - - Runs ``callback`` synchronously when nothing is detached, which is the - normal case — detaching only happens when a worker blows its join timeout. - - The gate is the whole registry, not "the workers reading through *this* - handle" — the registry doesn't track what each worker reads, and being - over-conservative here costs a delayed close, while being wrong costs a - read-after-close. The price is that one permanently wedged worker strands - every later callback too: each subsequent process switch then leaks its old - handle for the life of the app. - - That is the deliberate trade — a leaked handle over a read-after-close — so - the callback must be something a leak can survive (releasing a handle, not - freeing memory the app needs back). Callers that cannot wait for an event - loop want :func:`wait_for_detached_workers`, which is bounded instead. - """ - pending = [] - for worker in list(_DETACHED_WORKERS): - try: - if worker.isRunning(): - pending.append(worker) - except RuntimeError: - # Reaped and deleted between the snapshot and here — it can't be - # reading anything any more. Same tolerance as the loop below and - # as wait_for_detached_workers; letting it escape would abort a - # process switch mid-teardown. - continue - - if not pending: - callback() - return - - remaining = {"count": len(pending), "called": False} - - def _one_finished() -> None: - remaining["count"] -= 1 - if remaining["count"] <= 0 and not remaining["called"]: - remaining["called"] = True - callback() - - for worker in pending: - fired = {"yet": False} - - def _slot(*_args, fired=fired) -> None: - # Idempotent: `finished` and the isFinished() poll below can both - # reach this, and only the first arrival may count. - if fired["yet"]: - return - fired["yet"] = True - _one_finished() - - try: - worker.finished.connect(_slot) - # The worker may have finished between the isRunning() filter and - # the connect above, in which case `finished` already fired and - # nothing would ever call the slot. Poll once to cover that window. - done = worker.isFinished() - except RuntimeError: - # Reaped and deleted from under us: it cannot be reading any more, - # and leaving it uncounted would strand the callback forever. - done = True - if done: - _slot() - - class TearsDownOnClose: """Mixin that runs a dialog's teardown on *every* way out, not just close. @@ -193,10 +88,7 @@ class TearsDownOnClose: A dialog that stops its timers and joins its worker in ``closeEvent`` is therefore still polling after the user closed it — hidden, unreachable (the main window drops its reference on ``finished``), leaking a thread per - open/close cycle, and able to pop a modal from behind nothing. Worse for - this app: its worker never reaches :func:`shutdown_worker_thread`, so it is - absent from the detached registry that keeps a process handle alive while - something is still reading through it. + open/close cycle, and able to pop a modal from behind nothing. Subclasses implement :meth:`_teardown`; the mixin runs it exactly once, whichever way the dialog is dismissed. It must precede ``QDialog`` in the diff --git a/PyMemoryEditor/app/application.py b/PyMemoryEditor/app/application.py index 07cd645..bcc65ba 100644 --- a/PyMemoryEditor/app/application.py +++ b/PyMemoryEditor/app/application.py @@ -513,7 +513,6 @@ def main(argv=None): from .open_process_dialog import OpenProcessDialog from ._icon import app_icon - from ._widgets import wait_for_detached_workers app = QApplication.instance() or QApplication(argv) app.setApplicationName("PyMemoryEditor") @@ -539,20 +538,12 @@ def main(argv=None): try: app.exec() finally: - # A dialog fetch worker or the cheat poller that wouldn't stop in time - # was *detached* (see shutdown_worker_thread) and may still be reading - # through this handle. Closing it underneath one is a read-after-close, - # so wait for them first — and if one is still wedged, leave the handle - # to the OS, which reclaims it as this process exits anyway. There is no - # event loop left here, so this has to be the blocking wait rather than - # the signal-driven `call_when_detached_workers_finish`. - if wait_for_detached_workers(2000): - try: - # `process` is only the handle the picker opened; File → Change - # Process… released that one and attached the window to another. - window.process.close() - except Exception: - pass + try: + # `process` is only the handle the picker opened; File → Change + # Process… released that one and attached the window to another. + window.process.close() + except Exception: + pass def main_cli(argv=None): diff --git a/PyMemoryEditor/app/cheat_table.py b/PyMemoryEditor/app/cheat_table.py index 4627355..b784f28 100644 --- a/PyMemoryEditor/app/cheat_table.py +++ b/PyMemoryEditor/app/cheat_table.py @@ -109,9 +109,8 @@ def shutdown(self) -> None: iterations: draining queued writes and reading every cheat entry can outlast the wait on a big table or a hung target. That case goes through ``shutdown_worker_thread`` rather than being ignored, so the poller is - detached — kept alive away from this widget (which is about to be - deleted) and parked where ``call_when_detached_workers_finish`` can hold - the process handle open until it really is done reading. + *detached* — kept alive away from this widget instead of being deleted + with it, which would destroy a running QThread and abort the process. """ # Stop the publish timer first so no fresh snapshot races our shutdown. if self._publish_timer.isActive(): diff --git a/PyMemoryEditor/app/main_window.py b/PyMemoryEditor/app/main_window.py index bdca8a2..12694ce 100644 --- a/PyMemoryEditor/app/main_window.py +++ b/PyMemoryEditor/app/main_window.py @@ -46,7 +46,7 @@ from PyMemoryEditor import AbstractProcess, __version__ from ._icon import app_icon -from ._widgets import call_when_detached_workers_finish, detach_worker +from ._widgets import detach_worker from .application import DEFAULT_THEME_ID, THEMES, apply_theme from .cheat_entry import CheatEntry from .cheat_table import CheatTable @@ -893,21 +893,17 @@ def _change_process(self) -> None: old_cheat.shutdown() except Exception: pass - # The old poller has been stopped, but nothing here can *force* a thread - # wedged in a backend read to return: both the cheat poller and the - # dialog fetch workers fall back to being detached when they blow their - # join. Releasing the handle under one of those is a read-after-close (a - # recycled HANDLE on Windows, a deallocated Mach port on macOS), so hand - # the close to the helper that waits for every detached worker. Nothing - # detached — the normal case — closes inline. - - def _release_old_handle(process=old_process) -> None: - try: - process.close() - except Exception: - pass - - call_when_detached_workers_finish(_release_old_handle) + # The old poller has been stopped, so the handle can go. Caveat: nothing + # here can *force* a thread wedged in a backend read to return — the + # cheat poller and the dialog fetch workers fall back to being detached + # (see shutdown_worker_thread) when they blow their join, and one of + # those can still be mid-read when this closes. Harmless in practice: + # those fetches are read-only and their signals are already + # disconnected, so the backend errors out into a result nobody reads. + try: + old_process.close() + except Exception: + pass old_index = self._right_splitter.indexOf(old_cheat) self._cheat = CheatTable(self._process) self._cheat.pointer_scan_for_address.connect(self._open_pointer_scan_dialog) diff --git a/PyMemoryEditor/app/pointer_scan_dialog.py b/PyMemoryEditor/app/pointer_scan_dialog.py index cc60897..d16e4bb 100644 --- a/PyMemoryEditor/app/pointer_scan_dialog.py +++ b/PyMemoryEditor/app/pointer_scan_dialog.py @@ -626,7 +626,7 @@ def _start_worker(self, worker: QThread) -> None: worker.rows_ready.connect(self._append_rows) worker.error.connect(self._on_error) worker.finished_ok.connect(self._on_finished_ok) - worker.finished.connect(lambda w=worker: self._on_worker_finished(w)) + worker.finished.connect(self._on_worker_finished) self._worker = worker worker.start() @@ -736,22 +736,12 @@ def _on_finished_ok(self, count: int) -> None: self._count_label.setText(f"{shown:,} pointer path(s).") self._export_btn.setEnabled(shown > 0) - def _on_worker_finished(self, worker) -> None: - """Retire the scan that just finished — and only that one. - - No scan can currently be replaced while another is running (the Scan - button is disabled until this slot re-enables it), so the queued-signal - race that bites the polling dialogs can't happen here yet. Matching - their shape anyway: re-enabling the UI or dropping ``_worker`` on behalf - of a scan that was already superseded would be wrong the moment that - precondition changes. - """ - if worker is not self._worker: - worker.deleteLater() - return + def _on_worker_finished(self) -> None: + worker = self._worker self._worker = None self._set_scanning(False) - worker.deleteLater() + if worker is not None: + worker.deleteLater() # ----------------------------------------------------------- actions --- # diff --git a/tests/app/test_auto_refresh_dialog.py b/tests/app/test_auto_refresh_dialog.py index 28eb12a..4f5ea8b 100644 --- a/tests/app/test_auto_refresh_dialog.py +++ b/tests/app/test_auto_refresh_dialog.py @@ -323,19 +323,6 @@ def deleteLater(self): assert stale.deleted # and the one that really finished is retired -def test_an_explicit_refresh_reports_its_failure_immediately(qapp, make_dialog): - """A retry the user asked for answers now, not after the grace window.""" - dialog = make_dialog() # production grace: 3 s - dialog.failing = True - - dialog.refresh() - _spin(qapp, 200) - - assert len(dialog.errors) == 1 - assert dialog.gave_up == 0 # reported on its own merit, not by giving up - assert dialog._auto_timer.isActive() - - def test_nothing_fetches_after_the_dialog_was_torn_down(qapp, make_dialog): """The teardown latch is one-way, so a fetch started later is unjoinable.""" dialog = make_dialog() diff --git a/tests/app/test_detached_worker_close.py b/tests/app/test_detached_worker_close.py deleted file mode 100644 index ae77e09..0000000 --- a/tests/app/test_detached_worker_close.py +++ /dev/null @@ -1,189 +0,0 @@ -# -*- coding: utf-8 -*- - -""" -Tests for ``call_when_detached_workers_finish`` in ``PyMemoryEditor/app/_widgets.py``. - -``shutdown_worker_thread`` deliberately *detaches* a worker that blows its join -timeout instead of destroying it — otherwise the closing dialog would take a -running QThread down with it and abort the process. The consequence is a thread -that can still be inside a backend read after its dialog is gone, so whatever -releases the process handle it reads through (``MainWindow._change_process``) -has to wait for it rather than pull the ground out from under it. - -Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via -the ``app`` extra). -""" - -import os -import time - -import pytest - - -pytest.importorskip("PySide6", reason="App tests require PySide6 (install with [app] extra).") - -# Offscreen platform plugin: no display server needed, runs on CI. -os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") - - -@pytest.fixture(scope="module") -def qapp(): - """A single QApplication for the module (Qt allows only one per process).""" - from PySide6.QtWidgets import QApplication - - return QApplication.instance() or QApplication([]) - - -def _spin_until(qapp, predicate, timeout_ms=5000) -> bool: - """Pump the event loop until ``predicate`` holds (or the timeout expires).""" - from PySide6.QtCore import QElapsedTimer - - elapsed = QElapsedTimer() - elapsed.start() - while elapsed.elapsed() < timeout_ms: - if predicate(): - return True - qapp.processEvents() - time.sleep(0.005) - return predicate() - - -@pytest.fixture -def slow_worker(qapp): - """A worker that ignores the join timeout, so shutdown has to detach it.""" - from PySide6.QtCore import QThread - - class _WedgedWorker(QThread): - def __init__(self): - super().__init__() - self.busy_ms = 400 - # Python-side completion flag. Assertions must not call isRunning() - # after the worker finishes: the detach machinery reaps it (a - # deleteLater on the C++ object), and touching the wrapper then - # raises "Internal C++ object already deleted". - self.ran_to_completion = False - - def run(self): - # Stands in for a backend read that can't be interrupted — the only - # reason `shutdown_worker_thread` ever detaches anything. - QThread.msleep(self.busy_ms) - self.ran_to_completion = True - - workers = [] - - def _factory(): - worker = _WedgedWorker() - workers.append(worker) - return worker - - yield _factory - - for worker in workers: - try: - worker.wait(5000) - except RuntimeError: - # Already reaped: a detached worker deleteLater()s itself once it - # finishes, which leaves the Python wrapper pointing at nothing. - pass - qapp.processEvents() - - -def test_closes_inline_when_nothing_is_detached(qapp): - """The normal path: no wedged worker, so the handle is released right away.""" - from PyMemoryEditor.app._widgets import ( - call_when_detached_workers_finish, - wait_for_detached_workers, - ) - - # Precondition: nothing wedged from an earlier test. A finished worker may - # still sit in the registry waiting for its reaper, which doesn't count — - # and touching such a wrapper directly can hit an already-deleted C++ - # object, so ask through the guarded helper. - assert wait_for_detached_workers(0) is True - - closed = [] - call_when_detached_workers_finish(lambda: closed.append(True)) - assert closed == [True] # synchronous, not deferred to the event loop - - -def test_the_close_waits_for_a_detached_worker(qapp, slow_worker): - """A worker that outlived its join must not have the handle closed under it.""" - from PyMemoryEditor.app._widgets import ( - _DETACHED_WORKERS, - call_when_detached_workers_finish, - shutdown_worker_thread, - ) - - worker = slow_worker() - worker.start() - assert _spin_until(qapp, worker.isRunning, 2000) - - # A 0 ms join can't stop it, so shutdown detaches it — the situation - # _change_process has to survive. - shutdown_worker_thread(worker, wait_ms=0) - assert worker in _DETACHED_WORKERS - assert worker.isRunning() - - closed = [] - call_when_detached_workers_finish(lambda: closed.append(True)) - assert closed == [] # still reading — the handle must stay open - - assert _spin_until(qapp, lambda: bool(closed)) - assert closed == [True] - assert worker.ran_to_completion - - -def test_the_blocking_wait_reports_whether_the_workers_finished(qapp, slow_worker): - """App exit has no event loop left, so it needs the blocking sibling. - - ``QThread.wait`` works without a loop; ``finished`` would never be - delivered, so ``call_when_detached_workers_finish`` can't be used there. - """ - from PyMemoryEditor.app._widgets import ( - shutdown_worker_thread, - wait_for_detached_workers, - ) - - assert wait_for_detached_workers(0) is True # nothing detached - - worker = slow_worker() - worker.busy_ms = 600 - worker.start() - assert _spin_until(qapp, worker.isRunning, 2000) - shutdown_worker_thread(worker, wait_ms=0) - - # Too short: the caller must leave the handle alone rather than close it - # under a live read. - assert wait_for_detached_workers(50) is False - assert worker.isRunning() - - # Long enough: the worker is done, so the handle is safe to release. - assert wait_for_detached_workers(3000) is True - assert worker.ran_to_completion - - -def test_the_close_fires_once_for_several_detached_workers(qapp, slow_worker): - """Two wedged workers, one close — and only after the slower one is done.""" - from PyMemoryEditor.app._widgets import ( - call_when_detached_workers_finish, - shutdown_worker_thread, - ) - - quick, slow = slow_worker(), slow_worker() - quick.busy_ms, slow.busy_ms = 200, 800 - quick.start() - slow.start() - assert _spin_until(qapp, lambda: quick.isRunning() and slow.isRunning(), 2000) - - shutdown_worker_thread(quick, wait_ms=0) - shutdown_worker_thread(slow, wait_ms=0) - - closed = [] - call_when_detached_workers_finish(lambda: closed.append(True)) - - assert _spin_until(qapp, lambda: quick.ran_to_completion, 3000) - qapp.processEvents() - assert closed == [] # the other one is still reading - - assert _spin_until(qapp, lambda: bool(closed)) - assert closed == [True] # exactly once, not once per worker From 38cca7adab10c3370eff547d9dad94554872ef75 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 16:17:09 -0300 Subject: [PATCH 09/15] docs(app): fix a comment left stale by the scope trim The detach rationale still pointed at the handle-holding mechanism that the trim removed. The shared registry earns its place on its own: one place to look for threads that outlived their owner. --- PyMemoryEditor/app/main_window.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/PyMemoryEditor/app/main_window.py b/PyMemoryEditor/app/main_window.py index 12694ce..4d4a8a6 100644 --- a/PyMemoryEditor/app/main_window.py +++ b/PyMemoryEditor/app/main_window.py @@ -1010,10 +1010,9 @@ def _shutdown_worker(self) -> None: if worker.isRunning(): # Still wedged in a backend call. Detach it from this window so the # imminent window destruction can't destroy a live QThread, and keep - # it referenced until it finishes on its own. It goes in the shared - # registry, not a list of our own: a scan worker still reading is - # precisely what must hold the process handle open in - # _change_process and at app exit. + # it referenced until it finishes on its own — in the shared + # registry rather than a list of our own, so every thread that + # outlived its owner is in one place. detach_worker(worker) else: worker.deleteLater() From 8bb7cd35831445927320164dd1ec80417e9f0c76 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 17:14:39 -0300 Subject: [PATCH 10/15] fix(app): make shutdown_worker_thread actually disconnect the worker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `worker.disconnect()` — the bare, no-argument form — raises TypeError in PySide6 ("not enough arguments"), and the `except (RuntimeError, TypeError): pass` around it swallowed that. Nothing was ever disconnected: a slot still fired after the call returned. Every "a late emit can't touch a now-closing dialog" claim in this app rested on it, including the ones this branch added. `QObject.disconnect(worker, None, None, None)` is the form that works. `worker.disconnect(worker)` is not an alternative: it only drops connections whose *receiver* is the worker, and every slot here belongs to the owning dialog. Two consequences of the same root cause come with it: * `_handle_ready` / `_handle_failed` now refuse to act on a dismissed dialog, the way `refresh` / `_start_fetch` already did. Disconnecting cannot unqueue a delivery that was already posted, and during `MainWindow.closeEvent` that modal would be a nested event loop in the middle of teardown. * The Threads window waits for two consecutive empty enumerations before calling the target dead. An empty snapshot is legal-but-rare on a live process (`Thread32First` can come up dry), and the first fetch is reported the moment it fails — so a single blip announced the death of a process that was very much alive. Also corrects the `_change_process` comment about detached workers: the exposure is the concurrent read itself, not the discarded result, and that window is accepted rather than eliminated. --- PyMemoryEditor/app/_auto_refresh_dialog.py | 11 +++ PyMemoryEditor/app/_widgets.py | 10 ++- PyMemoryEditor/app/main_window.py | 8 +- PyMemoryEditor/app/threads_dialog.py | 18 ++++- tests/app/test_auto_refresh_dialog.py | 90 +++++++++++++++++++++- 5 files changed, 128 insertions(+), 9 deletions(-) diff --git a/PyMemoryEditor/app/_auto_refresh_dialog.py b/PyMemoryEditor/app/_auto_refresh_dialog.py index 1097314..e18540a 100644 --- a/PyMemoryEditor/app/_auto_refresh_dialog.py +++ b/PyMemoryEditor/app/_auto_refresh_dialog.py @@ -194,6 +194,8 @@ def _start_fetch(self) -> None: worker.start() def _handle_ready(self, data) -> None: + if getattr(self, "_teardown_done", False): + return self._has_data = True self._last_error = None self._failing_since = None @@ -222,7 +224,16 @@ def _handle_failed(self, message: str) -> None: modal's nested loop lets the poll keep running underneath it, and a target that fails on alternating ticks then stacks a fresh modal on each failure until the interpreter runs out of stack. + + Nothing is reported for a dialog the user already dismissed: the + teardown disconnects the worker, but a delivery already queued when + that happened must not raise a modal over a window that is gone (and + during ``MainWindow.closeEvent`` that modal would be a nested event + loop in the middle of teardown). """ + if getattr(self, "_teardown_done", False): + return + if self._failing_since is None: self._failing_since = QElapsedTimer() self._failing_since.start() diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index 1f0e104..b9ba745 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -8,7 +8,7 @@ from typing import Any, Callable, Iterable, List, Optional, Tuple -from PySide6.QtCore import Qt, QThread +from PySide6.QtCore import QObject, Qt, QThread from PySide6.QtGui import QStandardItem @@ -44,7 +44,13 @@ def shutdown_worker_thread(worker: Optional[QThread], wait_ms: int = 2000) -> No if callable(cancel): cancel() try: - worker.disconnect() # drop every outgoing connection at once + # The four-argument static form, not `worker.disconnect()`: the bare + # call raises TypeError in PySide6 ("not enough arguments"), and with + # the except below swallowing it nothing was ever disconnected — every + # slot still fired after this returned. `worker.disconnect(worker)` is + # no good either: it only drops connections whose *receiver* is the + # worker, and every slot here belongs to the owning dialog. + QObject.disconnect(worker, None, None, None) except (RuntimeError, TypeError): pass diff --git a/PyMemoryEditor/app/main_window.py b/PyMemoryEditor/app/main_window.py index 4d4a8a6..bd69345 100644 --- a/PyMemoryEditor/app/main_window.py +++ b/PyMemoryEditor/app/main_window.py @@ -897,9 +897,11 @@ def _change_process(self) -> None: # here can *force* a thread wedged in a backend read to return — the # cheat poller and the dialog fetch workers fall back to being detached # (see shutdown_worker_thread) when they blow their join, and one of - # those can still be mid-read when this closes. Harmless in practice: - # those fetches are read-only and their signals are already - # disconnected, so the backend errors out into a result nobody reads. + # those can still be *inside a read* when this closes. That window is + # accepted, not eliminated: the read itself is the exposure (a recycled + # HANDLE on Windows, a deallocated Mach port on macOS), and all that can + # be said for it is that the fetches are read-only and their results are + # disconnected, so nothing wrong reaches the UI. try: old_process.close() except Exception: diff --git a/PyMemoryEditor/app/threads_dialog.py b/PyMemoryEditor/app/threads_dialog.py index c542d27..bf052a9 100644 --- a/PyMemoryEditor/app/threads_dialog.py +++ b/PyMemoryEditor/app/threads_dialog.py @@ -43,6 +43,9 @@ def __init__(self, process: AbstractProcess, parent=None): # self-throttles if an enumeration takes longer than the interval. super().__init__(process, refresh_interval_ms=300, parent=parent) self._threads: List[ThreadInfo] = [] + # Consecutive empty enumerations (see _fetch_data). Touched only from + # the worker thread, one fetch at a time. + self._empty_streak = 0 self.setWindowTitle(f"Threads — PID {process.pid}") self.resize(640, 520) @@ -113,10 +116,19 @@ def _fetch_data(self): # where macOS raises. Without this the window would sit at "0 thread(s)" # against a dead target, never reporting and never giving up, while the # Memory Map and Modules windows next to it say the process is gone. + # + # Two in a row before saying so: an empty snapshot is legal-but-rare on + # Windows (Thread32First can come up dry on a live process), and the + # *first* fetch is reported the moment it fails — so a single blip would + # otherwise announce the death of a process that is very much alive. if not threads: - raise RuntimeError( - "The process reported no threads — it has most likely exited." - ) + self._empty_streak += 1 + if self._empty_streak >= 2: + raise RuntimeError( + "The process reported no threads — it has most likely exited." + ) + return threads + self._empty_streak = 0 return threads def _on_data_ready(self, threads) -> None: diff --git a/tests/app/test_auto_refresh_dialog.py b/tests/app/test_auto_refresh_dialog.py index 4f5ea8b..008f3a3 100644 --- a/tests/app/test_auto_refresh_dialog.py +++ b/tests/app/test_auto_refresh_dialog.py @@ -368,7 +368,9 @@ def get_threads(self): assert reported == [] state["threads"] = [] # the target exits - _spin(qapp, 600) + # 300 ms poll: one tick to see the empty list, another to call it, and + # the grace window on top before it is worth telling the user. + _spin(qapp, 1400) assert len(reported) == 1 assert "exited" in reported[0] @@ -377,6 +379,42 @@ def get_threads(self): _spin(qapp, 50) +def test_threads_dialog_tolerates_a_single_empty_enumeration(qapp, dialog_parent): + """One empty snapshot is not proof of death. + + ``Thread32First`` can legally come up dry on a live process, and the first + fetch is reported the moment it fails — so announcing the target's death on + a single blip would be a lie the user sees immediately. + """ + from PyMemoryEditor.app.threads_dialog import ThreadsDialog + from PyMemoryEditor.process.thread_info import ThreadInfo + + alive = [ThreadInfo(tid=1, start_address=None, state="S", priority=20, raw="1")] + state = {"threads": [], "calls": 0} + + class _Process: + pid = 4242 + + def get_threads(self): + state["calls"] += 1 + # Empty once, then healthy again. + return iter(state["threads"] if state["calls"] > 1 else []) + + reported = [] + dialog = ThreadsDialog(_Process(), parent=dialog_parent) + dialog._on_data_failed = lambda message: reported.append(message) + try: + state["threads"] = alive + _spin(qapp, 500) + + assert reported == [] # the blip healed, nobody was told anything + assert dialog._model.rowCount() == 1 + assert dialog._auto_timer.isActive() + finally: + dialog.close() + _spin(qapp, 50) + + def test_hex_viewer_stops_polling_and_logs_once_on_a_dead_target( qapp, dialog_parent, caplog, monkeypatch ): @@ -416,3 +454,53 @@ def read_process_memory(self, address, pytype, length): finally: viewer.close() _spin(qapp, 50) + + +def test_shutdown_really_disconnects_the_worker(qapp, dialog_parent, make_dialog): + """`worker.disconnect()` is a no-op in PySide6 — it raises TypeError. + + `shutdown_worker_thread` swallowed that, so every "a late emit can't touch a + dismissed dialog" claim in this app rested on a call that disconnected + nothing. The four-argument static form is the one that works for slots that + are bound methods of the owning dialog. + """ + from PyMemoryEditor.app._auto_refresh_dialog import _DataWorker + from PyMemoryEditor.app._widgets import shutdown_worker_thread + + dialog = make_dialog() + # Quiesce the dialog first: with its own poll running, a result already in + # flight would land during the pump below and move the counters for reasons + # that have nothing to do with this test. + dialog._auto_timer.stop() + _spin(qapp, 100) + rendered, errors = dialog.rendered, len(dialog.errors) + + worker = _DataWorker(lambda: ["x"], dialog) + worker.ready.connect(dialog._handle_ready) + worker.failed.connect(dialog._handle_failed) + + shutdown_worker_thread(worker, wait_ms=0) + worker.ready.emit(["late"]) + worker.failed.emit("late failure") + qapp.processEvents() + + assert dialog.rendered == rendered + assert len(dialog.errors) == errors + + +def test_a_dismissed_dialog_reports_nothing(qapp, make_dialog): + """Belt and braces for the same hazard, from the receiving end. + + A delivery already queued when the teardown ran must not pop a modal over a + window that is gone — during ``MainWindow.closeEvent`` that would be a + nested event loop in the middle of teardown. + """ + dialog = make_dialog() + dialog.accept() + _spin(qapp, 50) + + dialog._handle_failed("late failure") + dialog._handle_ready(["late"]) + + assert dialog.errors == [] + assert dialog.gave_up == 0 From 3c312a16431eda47f3770fd0aa3a6e692ed6ea42 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 17:31:26 -0300 Subject: [PATCH 11/15] fix(app): reap a worker that finishes as it is being detached `detach_worker` connects the reaper to `finished` after the caller has checked `isRunning()`. A worker that finishes inside that window has already emitted, so the reaper never runs and a dead worker stays parked in the registry for the life of the process. It now reaps on the spot when the thread turns out to be finished. Also gives the mixin an `_is_dismissed()` helper instead of repeating `getattr(self, "_teardown_done", False)` at four call sites: a typo in one copy would silently disable that guard, and the guards are what keep a dismissed dialog from fetching or reporting. --- PyMemoryEditor/app/_auto_refresh_dialog.py | 8 ++++---- PyMemoryEditor/app/_widgets.py | 17 ++++++++++++++++- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/PyMemoryEditor/app/_auto_refresh_dialog.py b/PyMemoryEditor/app/_auto_refresh_dialog.py index e18540a..94f19d2 100644 --- a/PyMemoryEditor/app/_auto_refresh_dialog.py +++ b/PyMemoryEditor/app/_auto_refresh_dialog.py @@ -156,7 +156,7 @@ def refresh(self) -> None: re-arming here would leave a timer running on a dialog nothing can join again. """ - if getattr(self, "_teardown_done", False): + if self._is_dismissed(): return self._last_error = None self._failing_since = None @@ -173,7 +173,7 @@ def _start_fetch(self) -> None: # Nothing may start a fetch after the dialog was dismissed: the teardown # latch is one-way, so a worker started here could never be joined # again (and its dialog is on its way to being deleted). - if getattr(self, "_teardown_done", False): + if self._is_dismissed(): return # Skip if a fetch is already in flight — the timer would otherwise stack @@ -194,7 +194,7 @@ def _start_fetch(self) -> None: worker.start() def _handle_ready(self, data) -> None: - if getattr(self, "_teardown_done", False): + if self._is_dismissed(): return self._has_data = True self._last_error = None @@ -231,7 +231,7 @@ def _handle_failed(self, message: str) -> None: during ``MainWindow.closeEvent`` that modal would be a nested event loop in the middle of teardown). """ - if getattr(self, "_teardown_done", False): + if self._is_dismissed(): return if self._failing_since is None: diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index b9ba745..25966a2 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -75,6 +75,11 @@ def detach_worker(worker: QThread) -> None: worker.setParent(None) _DETACHED_WORKERS.append(worker) worker.finished.connect(lambda: _reap_detached_worker(worker)) + # It can finish between the caller's isRunning() check and the connect + # above, in which case `finished` already fired and the reaper would never + # run — leaving a dead worker parked in the registry for good. + if worker.isFinished(): + _reap_detached_worker(worker) def _reap_detached_worker(worker: QThread) -> None: @@ -111,8 +116,18 @@ def _teardown(self) -> None: """Release timers and background threads. Runs exactly once.""" raise NotImplementedError + def _is_dismissed(self) -> bool: + """Whether the teardown has already run — i.e. the dialog is done. + + Callers guard their own entry points with this rather than repeating + the attribute lookup: a typo in one copy would silently disable that + guard, and the guards are what keep a dismissed dialog from fetching + or reporting. + """ + return getattr(self, "_teardown_done", False) + def _run_teardown_once(self) -> None: - if getattr(self, "_teardown_done", False): + if self._is_dismissed(): return self._teardown_done = True self._teardown() From 32c143376e5786a6329b4e754959fa0b9f27ffd4 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 18:00:54 -0300 Subject: [PATCH 12/15] style(app): trim the commentary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several of these blocks restated the code or told the story of how the bug was found. Kept the notes that would stop a future edit from reintroducing something — why the four-argument disconnect, why the clear() after closing hex viewers, why the snapshot is cleared last — and cut the rest. --- PyMemoryEditor/app/_auto_refresh_dialog.py | 117 +++++++-------------- PyMemoryEditor/app/_widgets.py | 52 +++------ PyMemoryEditor/app/cheat_table.py | 26 ++--- PyMemoryEditor/app/main_window.py | 58 ++++------ PyMemoryEditor/app/memory_map_dialog.py | 3 +- PyMemoryEditor/app/memory_viewer_dialog.py | 32 +++--- PyMemoryEditor/app/modules_dialog.py | 3 +- PyMemoryEditor/app/open_process_dialog.py | 21 ++-- PyMemoryEditor/app/threads_dialog.py | 21 ++-- tests/app/test_auto_refresh_dialog.py | 99 ++++++----------- 10 files changed, 146 insertions(+), 286 deletions(-) diff --git a/PyMemoryEditor/app/_auto_refresh_dialog.py b/PyMemoryEditor/app/_auto_refresh_dialog.py index 94f19d2..f9aae5e 100644 --- a/PyMemoryEditor/app/_auto_refresh_dialog.py +++ b/PyMemoryEditor/app/_auto_refresh_dialog.py @@ -11,10 +11,8 @@ copy wouldn't reach the others. This owns the lifecycle once; subclasses provide only what differs: how to fetch the data, and how to render it. -It also owns *when a failure is worth interrupting the user*, which a polling -dialog cannot get wrong quietly: ``_on_data_failed`` is a modal, a modal runs a -nested event loop, and a poll left running underneath one stacks a dialog per -tick (issue #74). See :meth:`AutoRefreshTableDialog._handle_failed`. +It also owns *when* a failure is worth interrupting the user — see +:meth:`AutoRefreshTableDialog._handle_failed`. """ from typing import Callable, Optional @@ -24,20 +22,11 @@ from ._widgets import TearsDownOnClose, shutdown_worker_thread -# How long a target may keep failing before the auto-refresh gives up. A -# failure is usually terminal — the target exited, or its handle was closed by -# File → Change Process — but not always: a module/thread enumeration can blip -# while the target loads images (``CreateToolhelp32Snapshot`` is documented to -# fail with ``ERROR_BAD_LENGTH`` mid-load), and giving up on the first blip -# would freeze a window that would have recovered on the next tick. -# -# Measured in wall-clock rather than ticks on purpose: "how long does the target -# get to recover" is the same question for every window, but the intervals are -# not (300 ms for Threads, 1000 ms for Memory Map / Modules, and anything from -# 50 ms up in the Hex Viewer, which shares this budget), so a tick count would -# hand them budgets differing by more than an order of magnitude. What gets *reported* is decided -# separately, in ``_handle_failed`` — this constant only bounds how long a -# failing target keeps a worker running. +# How long a target may keep failing before the auto-refresh gives up. Not +# every failure is terminal (``CreateToolhelp32Snapshot`` is documented to fail +# transiently while a target loads images), so a blip must not freeze a window +# that would recover next tick. In wall-clock, not ticks: the intervals differ +# by more than 10x across the windows that share this budget. _FAILURE_GRACE_MS = 3000 @@ -79,17 +68,12 @@ def __init__(self, process, *, refresh_interval_ms: int, parent=None): self._has_data = False self._refresh_interval_ms = refresh_interval_ms self._auto_timer: Optional[QTimer] = None - # Last error handed to ``_on_data_failed``, so a target that fails on - # every tick is reported once instead of once per tick. Cleared by a - # successful fetch or an explicit ``refresh()`` (see ``_handle_failed``). - self._last_error: Optional[str] = None - # Started on the first failure of a streak; None whenever the last - # fetch succeeded. Measures how long the target has been failing. - self._failing_since: Optional[QElapsedTimer] = None - # True from the moment the poll gives up until the next refresh(). Read - # by subclasses that rebuild their status line on user input (a filter - # keystroke), so the "stopped" note survives instead of being replaced - # by a count that looks live. + # Failure state, all reset by a success or an explicit refresh(). See + # _handle_failed for how they decide what reaches the user. + self._last_error: Optional[str] = None # doubles as "already reported" + self._failing_since: Optional[QElapsedTimer] = None # streak clock + # Read by subclasses that rebuild their status line on user input, so + # the "stopped" note isn't replaced by a count that looks live. self._polling_gave_up = False def _start_auto_refresh(self) -> None: @@ -121,10 +105,7 @@ def _on_data_failed(self, message: str) -> None: Called only for a failure the user needs to see — the first fetch, or the one that makes the poll give up — and never twice for the same - error (see :meth:`_handle_failed`), so a modal dialog is safe here. - A retry the user asks for while the window already has data waits for - that give-up rather than reporting on the spot; the status line still - carries the previous failure in the meantime. + error (see :meth:`_handle_failed`), so a modal is safe here. """ raise NotImplementedError @@ -134,10 +115,9 @@ def _set_loading_hint(self) -> None: def _on_polling_stopped(self) -> None: """Note that the auto-refresh gave up. Runs on the UI thread. - Fires once, when a target has been failing for ``_FAILURE_GRACE_MS`` - (see :meth:`_handle_failed`). The table is frozen on its last good data - from here on, so subclasses should say so — a silently stale window is - worse than an honest one. Optional. + Fires once, when a target has been failing for ``_FAILURE_GRACE_MS``. + The table is frozen on its last good data from here on, so subclasses + should say so. Optional. """ # ------------------------------------------------------------------ # @@ -147,14 +127,10 @@ def _on_polling_stopped(self) -> None: def refresh(self) -> None: """Fetch now, on the user's behalf. - Explicit (re)fetch: it clears the error latch and restarts a poll that - repeated failures stopped, so reopening the dialog against a live target - picks the cadence back up. The timer itself goes through - :meth:`_auto_refresh_tick` instead, which must *not* re-arm. - - A dismissed dialog stays dismissed: the teardown latch is one-way, so - re-arming here would leave a timer running on a dialog nothing can join - again. + Clears the error latch and restarts a poll that repeated failures + stopped. The timer goes through :meth:`_auto_refresh_tick` instead, + which must *not* re-arm — and neither may a dismissed dialog, whose + teardown latch is one-way. """ if self._is_dismissed(): return @@ -170,9 +146,7 @@ def _auto_refresh_tick(self) -> None: self._start_fetch() def _start_fetch(self) -> None: - # Nothing may start a fetch after the dialog was dismissed: the teardown - # latch is one-way, so a worker started here could never be joined - # again (and its dialog is on its way to being deleted). + # A worker started after the teardown could never be joined again. if self._is_dismissed(): return @@ -210,26 +184,17 @@ def _handle_failed(self, message: str) -> None: it is up and the failures stack — hundreds of focus-stealing dialogs the user could only escape by killing the app from another TTY (issue #74). - Reporting is therefore tied to the two moments where a failure actually - changes what the user sees: - - * the **first** fetch fails — the window is empty, so there is nothing - to look at but the error; - * the poll **gives up** after ``_FAILURE_GRACE_MS`` of continuous - failure — the table is frozen on stale data from here on. - - Everything in between heals quietly: the table keeps its last good rows - and the next tick usually recovers. That "quietly" is load-bearing, not - just tidy — reporting *every* failure re-opens the spam, because a - modal's nested loop lets the poll keep running underneath it, and a - target that fails on alternating ticks then stacks a fresh modal on each - failure until the interpreter runs out of stack. - - Nothing is reported for a dialog the user already dismissed: the - teardown disconnects the worker, but a delivery already queued when - that happened must not raise a modal over a window that is gone (and - during ``MainWindow.closeEvent`` that modal would be a nested event - loop in the middle of teardown). + Reporting is therefore tied to the two moments where a failure changes + what the user sees: the **first** fetch (empty window) and the one that + makes the poll **give up** (frozen table). Everything in between heals + quietly — and that silence is load-bearing: reporting every failure + re-opens the spam, since a target failing on alternating ticks stacks a + modal per failure until the stack runs out. + + A dismissed dialog reports nothing. The teardown disconnects the worker, + but a delivery already queued then must not raise a modal over a window + that is gone — during ``MainWindow.closeEvent`` that would be a nested + event loop in the middle of teardown. """ if self._is_dismissed(): return @@ -249,28 +214,22 @@ def _handle_failed(self, message: str) -> None: self._auto_timer.stop() self._polling_gave_up = True - # ``_last_error`` doubles as "already reported": it keeps a repeat of the - # same error quiet, and its None-ness marks a window that has never - # reported anything (cleared by a successful fetch and by refresh()). never_reported_yet = not self._has_data and self._last_error is None if (gave_up or never_reported_yet) and message != self._last_error: self._last_error = message self._on_data_failed(message) - # After the report, so the subclass's "stopped" note is what stays on - # screen once the user dismisses the modal. + # After the report, so the "stopped" note is what stays on screen. if gave_up: self._on_polling_stopped() def _on_worker_finished(self, worker) -> None: """Retire the worker that just finished — and only that one. - ``finished`` is queued, so it can arrive *after* the next tick already - started a replacement: a worker exits ``run()`` (``isRunning()`` goes - False, the guard in :meth:`_start_fetch` lets the next fetch through) - and its signal is delivered a moment later. Clearing ``self._worker`` - blindly there would drop the reference to the *running* replacement and - ``deleteLater()`` it — destroying a live QThread aborts the process. + ``finished`` is queued, so it can arrive after the next tick already + started a replacement. Clearing ``self._worker`` blindly would drop the + reference to that *running* worker and ``deleteLater()`` it — destroying + a live QThread aborts the process. """ if worker is self._worker: self._worker = None diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index 25966a2..5d50f61 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -44,12 +44,10 @@ def shutdown_worker_thread(worker: Optional[QThread], wait_ms: int = 2000) -> No if callable(cancel): cancel() try: - # The four-argument static form, not `worker.disconnect()`: the bare - # call raises TypeError in PySide6 ("not enough arguments"), and with - # the except below swallowing it nothing was ever disconnected — every - # slot still fired after this returned. `worker.disconnect(worker)` is - # no good either: it only drops connections whose *receiver* is the - # worker, and every slot here belongs to the owning dialog. + # The four-argument static form on purpose: bare `worker.disconnect()` + # raises TypeError in PySide6 (so this except swallowed it and nothing + # was disconnected), and `worker.disconnect(worker)` only drops + # connections whose *receiver* is the worker — ours belong to the dialog. QObject.disconnect(worker, None, None, None) except (RuntimeError, TypeError): pass @@ -68,16 +66,14 @@ def detach_worker(worker: QThread) -> None: Reparents it away from the dying widget and holds it in the module-level registry until it finishes on its own. Callers that stop workers their own - way (``MainWindow._shutdown_worker`` pumps the event loop for its blocking - connections) park them here too, so there is one place to look for threads - that outlived their owner rather than one list per caller. + way park them here too, so threads that outlived their owner are all in one + place. """ worker.setParent(None) _DETACHED_WORKERS.append(worker) worker.finished.connect(lambda: _reap_detached_worker(worker)) # It can finish between the caller's isRunning() check and the connect - # above, in which case `finished` already fired and the reaper would never - # run — leaving a dead worker parked in the registry for good. + # above; `finished` already fired, so the reaper would never run. if worker.isFinished(): _reap_detached_worker(worker) @@ -91,25 +87,16 @@ def _reap_detached_worker(worker: QThread) -> None: class TearsDownOnClose: """Mixin that runs a dialog's teardown on *every* way out, not just close. - ``QDialog`` delivers a ``QCloseEvent`` only for a real close — the window - manager's button, or an explicit ``close()``. This app's own "Close" buttons - call ``accept()`` and Esc reaches ``reject()``; both land in ``done()``, - which hides the dialog and emits ``finished`` **without** any close event. - - A dialog that stops its timers and joins its worker in ``closeEvent`` is - therefore still polling after the user closed it — hidden, unreachable - (the main window drops its reference on ``finished``), leaking a thread per - open/close cycle, and able to pop a modal from behind nothing. + ``QDialog`` delivers a ``QCloseEvent`` only for a real close. The app's own + "Close" buttons call ``accept()`` and Esc reaches ``reject()``; both land in + ``done()``, which hides the dialog and emits ``finished`` *without* any + close event — so a teardown living in ``closeEvent`` never ran, and the + dialog kept polling, hidden and unreachable. Subclasses implement :meth:`_teardown`; the mixin runs it exactly once, - whichever way the dialog is dismissed. It must precede ``QDialog`` in the - bases so its overrides win. - - Running *once* makes instances single-use, which is what every caller here - already assumes: the main window drops its reference on ``finished`` and - builds a fresh dialog next time. A dialog meant to be reshown after being - dismissed would have to re-arm what ``_teardown`` released rather than lean - on the latch. + whichever way the dialog is dismissed, and must precede ``QDialog`` in the + bases so its overrides win. Running once makes instances single-use, which + is what every caller here assumes. """ def _teardown(self) -> None: @@ -117,13 +104,8 @@ def _teardown(self) -> None: raise NotImplementedError def _is_dismissed(self) -> bool: - """Whether the teardown has already run — i.e. the dialog is done. - - Callers guard their own entry points with this rather than repeating - the attribute lookup: a typo in one copy would silently disable that - guard, and the guards are what keep a dismissed dialog from fetching - or reporting. - """ + """Whether the teardown has run. Guards live on this, not on a repeated + attribute lookup a typo could silently disable.""" return getattr(self, "_teardown_done", False) def _run_teardown_once(self) -> None: diff --git a/PyMemoryEditor/app/cheat_table.py b/PyMemoryEditor/app/cheat_table.py index b784f28..c2d16f9 100644 --- a/PyMemoryEditor/app/cheat_table.py +++ b/PyMemoryEditor/app/cheat_table.py @@ -73,8 +73,7 @@ def __init__(self, process: AbstractProcess, parent=None): self._process = process self._entries: List[CheatEntry] = [] self._suspend_signals = False - # Set before _build_ui(), which connects cellChanged: the guards that - # read this flag sit on paths that signal can reach. + # Before _build_ui(), which connects cellChanged — a path that reads it. self._poller_stopped = False self._build_ui() @@ -105,19 +104,17 @@ def shutdown(self) -> None: ``closeEvent`` need to drive the teardown explicitly to avoid leaving the poller running against a now-closed process handle. - The join is *bounded*, and a tick only checks the stop flag between - iterations: draining queued writes and reading every cheat entry can - outlast the wait on a big table or a hung target. That case goes through - ``shutdown_worker_thread`` rather than being ignored, so the poller is - *detached* — kept alive away from this widget instead of being deleted - with it, which would destroy a running QThread and abort the process. + The join is bounded and a tick only checks the stop flag between + iterations, so a big table or a hung target can outlast it. That case + goes through ``shutdown_worker_thread``, which *detaches* the poller + instead of letting it be deleted with this widget — destroying a running + QThread aborts the process. """ # Stop the publish timer first so no fresh snapshot races our shutdown. if self._publish_timer.isActive(): self._publish_timer.stop() # Idempotent: shutdown() is driven from several places, and handing the - # same worker to shutdown_worker_thread twice would touch an object the - # detach machinery may already have deleted. + # same worker over twice would touch an already-deleted object. if self._poller_stopped: return self._poller_stopped = True @@ -333,10 +330,7 @@ def _on_cell_changed(self, row: int, column: int) -> None: # next poll tick reads the value back and corrects the cell if the # write didn't take, and an outright failure returns via # write_failed → _on_write_failed. - # - # Nothing to route it to once shutdown() has handed the worker over: - # it is either deleted or detached and finishing, and this table is - # on its way out with the process handle it was writing through. + # Nothing to route it to once shutdown() handed the worker over. if self._poller_stopped: return self._poller.request_write( @@ -349,8 +343,8 @@ def _on_cell_changed(self, row: int, column: int) -> None: def _publish_snapshot_to_worker(self) -> None: """Hand the worker a fresh immutable snapshot of every entry.""" - # shutdown() stops the timer that drives this, but a tick already queued - # when it ran would otherwise reach a worker that is being torn down. + # A tick queued before shutdown() stopped the timer must not reach a + # worker being torn down. if self._poller_stopped: return snapshot = [ diff --git a/PyMemoryEditor/app/main_window.py b/PyMemoryEditor/app/main_window.py index bd69345..270f89a 100644 --- a/PyMemoryEditor/app/main_window.py +++ b/PyMemoryEditor/app/main_window.py @@ -761,13 +761,8 @@ def _process_badge_text(self) -> str: @property def process(self) -> AbstractProcess: - """The handle this window is currently attached to. - - Not necessarily the one it was constructed with: File → Change - Process… swaps it (and releases the old one), so anything outliving the - window — ``application.main``'s teardown — has to ask for it rather than - hold on to the handle it passed in. - """ + """The handle this window is currently attached to — not necessarily + the one it was constructed with, since File → Change Process… swaps it.""" return self._process def _window_title(self) -> str: @@ -854,9 +849,8 @@ def _change_process(self) -> None: # Tear down auxiliary dialogs that hold a reference to the old # process — reopening them rebuilds against the new target. The Memory - # Map belongs here too: it auto-refreshes on a timer, so leaving it open - # left it polling the handle closed just below — one of the two paths - # into the error-dialog spam of issue #74. + # Map belongs here too: it polls on a timer, so leaving it open left it + # reading the handle closed just below (issue #74). for dialog_attr in ( "_memory_map", "_threads_dialog", @@ -869,18 +863,15 @@ def _change_process(self) -> None: existing.close() setattr(self, dialog_attr, None) - # Hex viewers keep their own auto-refresh timer against the old handle. - # Each is WA_DeleteOnClose and drops itself from this list on `destroyed` - # — which fires when the deleteLater is processed, i.e. after this method - # returns. So the copy is belt-and-braces and the clear() is what - # actually empties the list; neither is redundant. + # Hex viewers poll the old handle too. They drop themselves from this + # list on `destroyed`, which only fires after this method returns — so + # the clear() is what actually empties it, not redundant. for viewer in list(self._hex_viewers): viewer.close() self._hex_viewers.clear() - # Cleared *after* the teardown above: closing the Memory Map emits - # ``finished``, and its handler adopts that dialog's last snapshot as - # the cached one — which belongs to the process we just left. + # Cleared *after* the teardown: closing the Memory Map makes its + # `finished` handler adopt that dialog's snapshot — the old target's. self._region_snapshot = None # Replace the cheat table — old entries point at the previous process. # QSplitter has no QLayout, so we use its native replaceWidget(index). @@ -893,15 +884,11 @@ def _change_process(self) -> None: old_cheat.shutdown() except Exception: pass - # The old poller has been stopped, so the handle can go. Caveat: nothing - # here can *force* a thread wedged in a backend read to return — the - # cheat poller and the dialog fetch workers fall back to being detached - # (see shutdown_worker_thread) when they blow their join, and one of - # those can still be *inside a read* when this closes. That window is - # accepted, not eliminated: the read itself is the exposure (a recycled - # HANDLE on Windows, a deallocated Mach port on macOS), and all that can - # be said for it is that the fetches are read-only and their results are - # disconnected, so nothing wrong reaches the UI. + # The old poller has been stopped, so the handle can go. Accepted + # window: a worker that blew its join is *detached*, not stopped, so it + # can still be inside a read here (a recycled HANDLE on Windows, a + # deallocated Mach port on macOS). The fetches are read-only and their + # results disconnected, so nothing wrong reaches the UI. try: old_process.close() except Exception: @@ -949,12 +936,10 @@ def closeEvent(self, event: QCloseEvent) -> None: if dialog is not None: dialog.close() - # Hex viewers run the same kind of read worker and are children of this - # window, so destroying it would take a running QThread down with it. - # No clear() here (unlike _change_process, which keeps running against a - # new target afterwards): this window is on its way out with the list. - # (The Pointer Chain dialog is deliberately absent from both lists: it - # resolves synchronously and starts no thread.) + # Hex viewers run the same read worker and are children of this window, + # so destroying it would take a running QThread down with it. No clear() + # here: the list dies with the window. (Pointer Chain is absent from + # both lists on purpose — it resolves synchronously, no thread.) for viewer in list(self._hex_viewers): viewer.close() @@ -1010,11 +995,8 @@ def _shutdown_worker(self) -> None: waited += step_ms if worker.isRunning(): - # Still wedged in a backend call. Detach it from this window so the - # imminent window destruction can't destroy a live QThread, and keep - # it referenced until it finishes on its own — in the shared - # registry rather than a list of our own, so every thread that - # outlived its owner is in one place. + # Still wedged in a backend call: park it in the shared registry so + # the imminent window destruction can't take a live QThread down. detach_worker(worker) else: worker.deleteLater() diff --git a/PyMemoryEditor/app/memory_map_dialog.py b/PyMemoryEditor/app/memory_map_dialog.py index a0be6bf..59f304e 100644 --- a/PyMemoryEditor/app/memory_map_dialog.py +++ b/PyMemoryEditor/app/memory_map_dialog.py @@ -423,8 +423,7 @@ def _populate(self) -> None: f"{total:,} regions · " f"{_format_size(total_bytes)} of virtual address space mapped" ) - # A filter keystroke rebuilds this line long after the poll gave up — - # without the suffix the stale table would look live again. + # A filter keystroke rebuilds this line long after the poll gave up. if self._polling_gave_up: count_text += " · auto-refresh stopped" self._count_label.setText(count_text) diff --git a/PyMemoryEditor/app/memory_viewer_dialog.py b/PyMemoryEditor/app/memory_viewer_dialog.py index ac3d973..b28f21f 100644 --- a/PyMemoryEditor/app/memory_viewer_dialog.py +++ b/PyMemoryEditor/app/memory_viewer_dialog.py @@ -146,15 +146,10 @@ def _build_ui(self) -> None: self._timer = QTimer(self) self._timer.timeout.connect(self.refresh) - # Failure bookkeeping, same shape as the auto-refresh dialogs: report a - # given error once rather than once per tick, and stop polling a target - # that keeps failing. Without it a dead target logged a warning on every - # tick forever — up to 20 a second at the 50 ms floor — which is the - # spam of issue #74 wearing a different hat (the Log Console instead of - # a modal). - # Keyed by (address, size, message): the point is to stop repeating the - # same line every tick, not to go quiet about a *different* range that - # started failing after the user moved the viewer. + # Failure bookkeeping, same shape as the auto-refresh dialogs: a dead + # target used to log a warning every tick forever (~20/s at the 50 ms + # floor). Keyed by (address, size, message) so moving the viewer to + # another failing range still gets logged. self._last_failure: Optional[tuple] = None self._failing_since: Optional[QElapsedTimer] = None @@ -218,7 +213,7 @@ def _on_read_result(self, result) -> None: self._dump.setPlainText("") message = f"{type(exc).__name__}: {exc}" - # Log the first tick of a streak, not every one of them. + # First tick of a streak only. if (addr, size, message) != self._last_failure: self._last_failure = (addr, size, message) _LOG.warning( @@ -236,9 +231,8 @@ def _on_read_result(self, result) -> None: self._failing_since.elapsed() >= _FAILURE_GRACE_MS and self._auto_btn.isChecked() ): - # Turning the button off stops the timer through the normal - # slot, so the UI can't claim to be refreshing while it isn't — - # and the user re-enables it with the same button. + # Off through the button's own slot, so the UI can't claim to + # be refreshing while it isn't. gave_up = True self._auto_btn.setChecked(False) @@ -256,11 +250,10 @@ def _on_read_result(self, result) -> None: def _on_worker_finished(self, worker) -> None: """Retire the worker that just finished — and only that one. - ``finished`` is queued and can land after the next auto-refresh tick - started a replacement (the interval goes down to 50 ms here, so the two - overlap easily). Clearing ``self._worker`` blindly would drop the - reference to the running replacement and ``deleteLater()`` it — - destroying a live QThread aborts the process. + ``finished`` is queued and can land after the next tick started a + replacement (50 ms interval floor here, so they overlap easily). + Clearing ``self._worker`` blindly would ``deleteLater()`` that running + worker — destroying a live QThread aborts the process. """ if worker is self._worker: self._worker = None @@ -269,8 +262,7 @@ def _on_worker_finished(self, worker) -> None: def _toggle_auto(self, on: bool) -> None: self._auto_btn.setText("Auto-refresh: On" if on else "Auto-refresh: Off") if on: - # Turning it back on is the user saying "try again": give the target - # a fresh grace window instead of one already spent. + # Turning it back on is a retry: fresh grace window. self._failing_since = None self._sync_timer() else: diff --git a/PyMemoryEditor/app/modules_dialog.py b/PyMemoryEditor/app/modules_dialog.py index b5b85e9..aacf8a3 100644 --- a/PyMemoryEditor/app/modules_dialog.py +++ b/PyMemoryEditor/app/modules_dialog.py @@ -193,8 +193,7 @@ def _apply_filter(self) -> None: count_text = f"{shown:,} of {total:,} module(s) shown" else: count_text = f"{total:,} module(s)" - # A filter keystroke rebuilds this line long after the poll gave up — - # without the suffix the stale table would look live again. + # A filter keystroke rebuilds this line long after the poll gave up. if self._polling_gave_up: count_text += " · auto-refresh stopped" self._count_label.setText(count_text) diff --git a/PyMemoryEditor/app/open_process_dialog.py b/PyMemoryEditor/app/open_process_dialog.py index 2d70902..dcdc2c5 100644 --- a/PyMemoryEditor/app/open_process_dialog.py +++ b/PyMemoryEditor/app/open_process_dialog.py @@ -324,12 +324,9 @@ def _on_rows_ready(self, rows) -> None: def _on_scan_finished(self, worker) -> None: """Retire the enumeration that just finished — and only that one. - ``finished`` is queued, so it can land after a refresh already started a - replacement (the worker leaves ``run()`` before its signal is - delivered, and the in-flight guard in ``_populate_processes`` lets the - next scan through in that window). Clearing ``_scan_worker`` blindly - would drop the reference to the running replacement and - ``deleteLater()`` it, which aborts the process. + ``finished`` is queued and can land after a refresh started a + replacement; clearing ``_scan_worker`` blindly would ``deleteLater()`` + that running worker, which aborts the process. """ if worker is self._scan_worker: self._scan_worker = None @@ -337,15 +334,9 @@ def _on_scan_finished(self, worker) -> None: def _teardown(self) -> None: self._refresh_timer.stop() - # Hand the enumeration worker to the shared shutdown: it disconnects, - # joins, and — when psutil is slow on a huge process list — detaches - # instead of leaving a live QThread parented to a dialog that is about - # to be dropped (destroying one aborts the process). - # - # The join is deliberately short. This runs on the click that dismisses - # the picker, and its result is already irrelevant: whoever waits is - # waiting for a process list nobody will read. Long enough to absorb a - # scan that is nearly done, short enough to be imperceptible. + # Short join on purpose: this runs on the click that dismisses the + # picker, waiting for a process list nobody will read. A scan that + # outlasts it is detached rather than left parented to a dying dialog. shutdown_worker_thread(self._scan_worker, wait_ms=250) self._scan_worker = None diff --git a/PyMemoryEditor/app/threads_dialog.py b/PyMemoryEditor/app/threads_dialog.py index bf052a9..738e635 100644 --- a/PyMemoryEditor/app/threads_dialog.py +++ b/PyMemoryEditor/app/threads_dialog.py @@ -43,9 +43,7 @@ def __init__(self, process: AbstractProcess, parent=None): # self-throttles if an enumeration takes longer than the interval. super().__init__(process, refresh_interval_ms=300, parent=parent) self._threads: List[ThreadInfo] = [] - # Consecutive empty enumerations (see _fetch_data). Touched only from - # the worker thread, one fetch at a time. - self._empty_streak = 0 + self._empty_streak = 0 # see _fetch_data; worker thread only self.setWindowTitle(f"Threads — PID {process.pid}") self.resize(640, 520) @@ -110,17 +108,12 @@ def _set_loading_hint(self) -> None: def _fetch_data(self): threads = list(self._process.get_threads()) - # A live process always has at least one thread, and `get_threads` does - # not report an exited one uniformly: Linux walks /proc//task and - # Windows filters a system-wide snapshot, so both simply come up empty - # where macOS raises. Without this the window would sit at "0 thread(s)" - # against a dead target, never reporting and never giving up, while the - # Memory Map and Modules windows next to it say the process is gone. - # - # Two in a row before saying so: an empty snapshot is legal-but-rare on - # Windows (Thread32First can come up dry on a live process), and the - # *first* fetch is reported the moment it fails — so a single blip would - # otherwise announce the death of a process that is very much alive. + # A live process always has a thread, but `get_threads` reports an + # exited one as an empty list on Linux/Windows (only macOS raises) — + # so the window would sit at "0 thread(s)" forever while Memory Map and + # Modules say the process is gone. Two in a row before saying so: an + # empty snapshot is legal-but-rare on a live process, and the first + # fetch is reported the moment it fails. if not threads: self._empty_streak += 1 if self._empty_streak >= 2: diff --git a/tests/app/test_auto_refresh_dialog.py b/tests/app/test_auto_refresh_dialog.py index 008f3a3..67cab35 100644 --- a/tests/app/test_auto_refresh_dialog.py +++ b/tests/app/test_auto_refresh_dialog.py @@ -3,18 +3,13 @@ """ Tests for ``PyMemoryEditor/app/_auto_refresh_dialog.py``. -Regression cover for issue #74: the Memory Map / Modules / Threads dialogs poll -the target every 300–1000 ms and report a failed fetch with a *modal* dialog. -Once the target exits (or its handle is closed by File → Change Process) every -tick fails, and because a modal runs a nested event loop the timer keeps firing -while it's up — the app ended up with hundreds of stacked, focus-stealing error -boxes and had to be killed from another terminal. +Regression cover for issue #74: these dialogs poll every 300–1000 ms and used to +report every failed fetch with a modal. A modal runs a nested event loop, so the +timer kept firing underneath and the dialogs stacked — hundreds of them. The contract now: a failure is reported only when it changes what the user sees -— the first fetch (empty window) or the one that makes the poll give up after -``_FAILURE_GRACE_MS`` of continuous failure (frozen table) — and never twice for -the same error. Failures in between heal quietly, which is what keeps a target -failing on alternating ticks from stacking a modal per failure. +— the first fetch (empty window) or the one that makes the poll give up (frozen +table) — and never twice for the same error. Skipped when ``PySide6`` isn't installed (the runtime dependency is opt-in via the ``app`` extra). @@ -64,15 +59,10 @@ def short_grace(monkeypatch): def dialog_parent(qapp): """A hidden parent, so Qt owns the dialogs instead of Python's GC. - A parentless QDialog is owned by its Python wrapper: it is destroyed the - instant the test's reference goes away, taking its worker children with it - while the event queue may still hold events for them. That left the process - crashing later — a segfault inside a *different* test's ``qtbot.wait``. - Parenting them here keeps every object alive until the module is done, so - nothing is freed underneath a queued event. - - Deliberately never deleted: the process is about to exit anyway, and a - teardown here would recreate the very window this avoids. + A parentless QDialog dies with the test's reference, taking its worker + children with it while the event queue may still hold events for them — + which segfaulted a *later* test's ``qtbot.wait``. Never deleted on purpose: + a teardown here would recreate that window. """ from PySide6.QtWidgets import QWidget @@ -83,9 +73,8 @@ def dialog_parent(qapp): def make_dialog(qapp, dialog_parent): """Factory for an ``AutoRefreshTableDialog`` whose fetch can be made to fail. - Dialogs are closed at the end of each test — that is what exercises the - teardown — but stay owned by ``dialog_parent`` (see there) rather than being - destroyed mid-session. + Closed at the end of each test (that exercises the teardown) but owned by + ``dialog_parent``, not destroyed mid-session. """ from PyMemoryEditor.app._auto_refresh_dialog import AutoRefreshTableDialog @@ -171,10 +160,9 @@ def test_polling_stops_when_failures_persist(qapp, short_grace, make_dialog): def test_a_flapping_target_never_stacks_dialogs(qapp, make_dialog): """Fail / succeed / fail / succeed must stay silent, not pop a modal a tick. - The regression this guards: reporting every failure looked safe because the - same error is never reported twice in a row — but each success cleared that - latch, so an alternating target popped a modal per failure, each one nesting - an event loop inside the previous modal until the stack blew up. + Reporting every failure looked safe because the same error is never reported + twice — but each success cleared that latch, so an alternating target popped + a modal per failure, nesting event loops until the stack blew up. """ dialog = make_dialog() dialog.flapping = True @@ -198,8 +186,7 @@ def test_the_first_fetch_failure_is_reported_immediately(qapp, make_dialog): def test_failures_inside_the_grace_window_are_not_reported(make_dialog): """A blip (a snapshot that fails while the target loads images) stays quiet. - Driven through the handlers rather than the timer: what matters is the - failure *sequence*, and racing a 50 ms poll to land an exact number of + Driven through the handlers: racing a 50 ms poll to land an exact number of failures would only make the test flaky. """ dialog = make_dialog() @@ -241,10 +228,8 @@ def test_the_close_button_tears_the_dialog_down(qapp, make_dialog): """The Close button calls accept(), which delivers no QCloseEvent. The teardown used to live in ``closeEvent`` alone, so dismissing a polling - dialog the normal way left it hidden and *still polling* — unreachable - (the main window drops its reference on ``finished``), leaking a thread per - open/close cycle, and with its worker absent from the detached registry that - keeps the process handle alive while something is still reading. + dialog the normal way left it hidden and *still polling*, leaking a thread + per open/close cycle. """ dialog = make_dialog() assert dialog._auto_timer.isActive() @@ -262,9 +247,9 @@ def test_the_close_button_tears_the_dialog_down(qapp, make_dialog): def test_escape_tears_the_dialog_down_too(qapp, make_dialog): """Esc reaches reject(), the other half of done(). - Driven as a real key event rather than a ``reject()`` call: Esc is handled - inside ``QDialog`` on the C++ side, so this is what proves the override is - reached through Qt's virtual dispatch and not just from Python. + A real key event, not a ``reject()`` call: Esc is handled inside ``QDialog`` + on the C++ side, so this proves the override is reached through Qt's virtual + dispatch. """ from PySide6.QtCore import Qt from PySide6.QtTest import QTest @@ -298,11 +283,10 @@ def test_teardown_runs_once_however_the_dialog_is_dismissed(qapp, make_dialog): def test_a_late_finished_does_not_retire_the_worker_that_replaced_it(make_dialog): - """``finished`` is queued and can land after the next tick started a new fetch. + """``finished`` is queued and can land after the next tick started a fetch. - Retiring ``self._worker`` blindly there dropped the reference to the - *running* replacement and scheduled it for deletion — destroying a live - QThread aborts the process, and the teardown could no longer join it. + Retiring ``self._worker`` blindly dropped the reference to the *running* + replacement and scheduled it for deletion — that aborts the process. """ dialog = make_dialog() dialog._start_fetch() @@ -341,12 +325,8 @@ def test_nothing_fetches_after_the_dialog_was_torn_down(qapp, make_dialog): def test_threads_dialog_treats_an_empty_enumeration_as_a_dead_target( qapp, short_grace, dialog_parent ): - """Linux and Windows report an exited process as *no threads*, not an error. - - Without this the window would sit at "0 thread(s)" against a dead target — - never reporting, never giving up — while the Memory Map and Modules windows - beside it say the process is gone. - """ + """Linux and Windows report an exited process as *no threads*, not an error, + so the window would sit at "0 thread(s)" forever.""" from PyMemoryEditor.app.threads_dialog import ThreadsDialog from PyMemoryEditor.process.thread_info import ThreadInfo @@ -380,12 +360,8 @@ def get_threads(self): def test_threads_dialog_tolerates_a_single_empty_enumeration(qapp, dialog_parent): - """One empty snapshot is not proof of death. - - ``Thread32First`` can legally come up dry on a live process, and the first - fetch is reported the moment it fails — so announcing the target's death on - a single blip would be a lie the user sees immediately. - """ + """One empty snapshot is not proof of death: ``Thread32First`` can come up + dry on a live process, and the first fetch is reported immediately.""" from PyMemoryEditor.app.threads_dialog import ThreadsDialog from PyMemoryEditor.process.thread_info import ThreadInfo @@ -420,10 +396,8 @@ def test_hex_viewer_stops_polling_and_logs_once_on_a_dead_target( ): """The Hex Viewer is the other polling window, and it never gave up. - Its reads fail every tick against a dead target, and each one logged a - warning — up to 20 a second at the 50 ms floor, straight into the Log - Console. Same defect as issue #74, reported through the logger instead of a - modal. + Against a dead target every tick logged a warning — up to 20 a second at the + 50 ms floor, straight into the Log Console. """ import logging @@ -459,10 +433,9 @@ def read_process_memory(self, address, pytype, length): def test_shutdown_really_disconnects_the_worker(qapp, dialog_parent, make_dialog): """`worker.disconnect()` is a no-op in PySide6 — it raises TypeError. - `shutdown_worker_thread` swallowed that, so every "a late emit can't touch a - dismissed dialog" claim in this app rested on a call that disconnected - nothing. The four-argument static form is the one that works for slots that - are bound methods of the owning dialog. + `shutdown_worker_thread` swallowed that, so nothing was ever disconnected. + The four-argument static form is the one that works for slots that are bound + methods of the owning dialog. """ from PyMemoryEditor.app._auto_refresh_dialog import _DataWorker from PyMemoryEditor.app._widgets import shutdown_worker_thread @@ -489,12 +462,8 @@ def test_shutdown_really_disconnects_the_worker(qapp, dialog_parent, make_dialog def test_a_dismissed_dialog_reports_nothing(qapp, make_dialog): - """Belt and braces for the same hazard, from the receiving end. - - A delivery already queued when the teardown ran must not pop a modal over a - window that is gone — during ``MainWindow.closeEvent`` that would be a - nested event loop in the middle of teardown. - """ + """The same hazard from the receiving end: a delivery already queued when + the teardown ran must not pop a modal over a window that is gone.""" dialog = make_dialog() dialog.accept() _spin(qapp, 50) From eda1c1fdc72d0c3692871b11555565d22f252c90 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 18:39:32 -0300 Subject: [PATCH 13/15] fix(app): close the polling dialogs before the teardown pumps the loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MainWindow.closeEvent` ran `_shutdown_worker()` — which pumps the event loop for up to two seconds while it waits on the scan worker — before closing the auxiliary dialogs. The dialogs are still live during that pump, so a poll landing there reaches `_handle_failed` and pops a modal in the middle of teardown. The heartbeat was already stopped first for exactly this reason; the dialogs are the same hazard and were missed. Reproduced with Modules open past its grace window and a wedged scan worker: one modal during the pump before, none after. Three smaller ones from the same review: * The cheat poller checks the stop flag per entry rather than once per tick. It is the worker that *writes*, and a write landing on a handle closed behind it is far worse than a stale read — a recycled HANDLE on Windows sends it into an unrelated process. Blowing the join now takes a single syscall hanging for a second, not a large table. * `detach_worker` sweeps finished workers out of the registry. Qt emits `finished` before `isFinished()` flips, so the check added for that race could miss it and leave a dead worker parked for good. * `MemoryViewerDialog._on_read_result` and `PointerScanDialog._on_error` refuse to act on a dismissed dialog, like the auto-refresh handlers already did. Disconnecting cannot unqueue a delivery already posted, and `_on_error` pops a modal. --- PyMemoryEditor/app/_widgets.py | 13 +++++-- PyMemoryEditor/app/cheat_poll_worker.py | 12 +++++++ PyMemoryEditor/app/main_window.py | 41 ++++++++++++---------- PyMemoryEditor/app/memory_viewer_dialog.py | 3 ++ PyMemoryEditor/app/pointer_scan_dialog.py | 4 +++ 5 files changed, 52 insertions(+), 21 deletions(-) diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index 5d50f61..9cf241b 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -69,11 +69,20 @@ def detach_worker(worker: QThread) -> None: way park them here too, so threads that outlived their owner are all in one place. """ + # Opportunistic sweep: a worker that finished between its caller's + # isRunning() check and the connect below never reaches its reaper, and Qt + # emits `finished` before isFinished() flips — so the check afterwards can + # miss it too. Pruning here keeps those from piling up. + for parked in list(_DETACHED_WORKERS): + try: + if parked.isFinished(): + _reap_detached_worker(parked) + except RuntimeError: # already deleted + _DETACHED_WORKERS.remove(parked) + worker.setParent(None) _DETACHED_WORKERS.append(worker) worker.finished.connect(lambda: _reap_detached_worker(worker)) - # It can finish between the caller's isRunning() check and the connect - # above; `finished` already fired, so the reaper would never run. if worker.isFinished(): _reap_detached_worker(worker) diff --git a/PyMemoryEditor/app/cheat_poll_worker.py b/PyMemoryEditor/app/cheat_poll_worker.py index 0abcda1..9c0b153 100644 --- a/PyMemoryEditor/app/cheat_poll_worker.py +++ b/PyMemoryEditor/app/cheat_poll_worker.py @@ -106,6 +106,11 @@ def request_write( with QMutexLocker(self._mutex): self._pending_writes.append((address, pytype, length, value)) + def _stopping(self) -> bool: + """Whether ``stop`` was asked for. Cheap enough to check per entry.""" + with QMutexLocker(self._mutex): + return self._stop + def stop(self) -> None: with QMutexLocker(self._mutex): self._stop = True @@ -126,6 +131,8 @@ def _drain_pending_writes(self) -> List[Tuple[int, type, int, str]]: failures: List[Tuple[int, type, int, str]] = [] for address, pytype, length, value in pending: + if self._stopping(): + break # same reason as _poll_once: the handle is about to go try: self._process.write_process_memory(address, pytype, length, value) except Exception as exc: # noqa: BLE001 @@ -206,6 +213,11 @@ def _poll_once( values_by_address = None for address in addresses: + # Checked per entry, not once per tick: every entry is a syscall + # and shutdown() closes the handle shortly after stop(), so a + # write landing on a recycled handle would hit another process. + if self._stopping(): + return results frozen_value, is_frozen = freeze_by_addr[(pytype, length, address)] if values_by_address is not None: current = values_by_address.get(address) diff --git a/PyMemoryEditor/app/main_window.py b/PyMemoryEditor/app/main_window.py index 270f89a..2848c82 100644 --- a/PyMemoryEditor/app/main_window.py +++ b/PyMemoryEditor/app/main_window.py @@ -885,10 +885,12 @@ def _change_process(self) -> None: except Exception: pass # The old poller has been stopped, so the handle can go. Accepted - # window: a worker that blew its join is *detached*, not stopped, so it - # can still be inside a read here (a recycled HANDLE on Windows, a - # deallocated Mach port on macOS). The fetches are read-only and their - # results disconnected, so nothing wrong reaches the UI. + # window: a worker that blew its join is *detached*, not stopped, and + # can still be inside a syscall here (a recycled HANDLE on Windows, a + # deallocated Mach port on macOS). The cheat poller is the one that + # *writes*, which is why it now checks the stop flag per entry rather + # than per tick — blowing the join takes a single syscall hanging for a + # second, not a slow table. try: old_process.close() except Exception: @@ -911,21 +913,11 @@ def closeEvent(self, event: QCloseEvent) -> None: # dialog mid-teardown. self._heartbeat.stop() - # The cheat poller is a child widget but its closeEvent is *not* - # guaranteed to fire on a top-level window close — only on an explicit - # close of the child. Drive it directly so the QThread is stopped - # before the process handle goes away in `application.main`'s finally. - try: - self._cheat.shutdown() - except Exception: - pass - - self._shutdown_worker() - - # Close worker-bearing auxiliary dialogs so their own closeEvent stops - # and joins the background thread. Otherwise the imminent destruction of - # this window would destroy those still-running QThreads and abort the - # process ("QThread: Destroyed while thread is still running"). + # Dialogs next, and *before* _shutdown_worker: it pumps the event + # loop, and a dialog still polling during that pump can reach + # _handle_failed and pop a modal mid-teardown — the same reason the + # heartbeat is stopped first. Closing them also joins their threads, + # which this window's destruction would otherwise take down with it. for attr in ( "_memory_map", "_threads_dialog", @@ -943,6 +935,17 @@ def closeEvent(self, event: QCloseEvent) -> None: for viewer in list(self._hex_viewers): viewer.close() + # The cheat poller is a child widget but its closeEvent is *not* + # guaranteed to fire on a top-level window close — only on an explicit + # close of the child. Drive it directly so the QThread is stopped + # before the process handle goes away in `application.main`'s finally. + try: + self._cheat.shutdown() + except Exception: + pass + + self._shutdown_worker() + self.closing.emit() super().closeEvent(event) diff --git a/PyMemoryEditor/app/memory_viewer_dialog.py b/PyMemoryEditor/app/memory_viewer_dialog.py index b28f21f..3e61d70 100644 --- a/PyMemoryEditor/app/memory_viewer_dialog.py +++ b/PyMemoryEditor/app/memory_viewer_dialog.py @@ -208,6 +208,9 @@ def fetch(): def _on_read_result(self, result) -> None: """Render a finished read (UI thread). ``result`` is the fetch tuple.""" + # A metacall queued before _teardown disconnected still gets delivered. + if self._is_dismissed(): + return addr, size, data, exc = result if exc is not None: self._dump.setPlainText("") diff --git a/PyMemoryEditor/app/pointer_scan_dialog.py b/PyMemoryEditor/app/pointer_scan_dialog.py index d16e4bb..ea6c58c 100644 --- a/PyMemoryEditor/app/pointer_scan_dialog.py +++ b/PyMemoryEditor/app/pointer_scan_dialog.py @@ -721,6 +721,10 @@ def _append_rows(self, rows: List[_ResultRow]) -> None: ) def _on_error(self, message: str) -> None: + # A metacall queued before _teardown disconnected still gets delivered, + # and a modal over a dismissed dialog is the hazard this PR is about. + if self._is_dismissed(): + return QMessageBox.critical(self, "Pointer Scan", message) self._count_label.setText("Scan failed.") From f0624399a9e46f2e17afced47b7bf75d5653f882 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 18:43:04 -0300 Subject: [PATCH 14/15] style(app): trim the commentary again Same pass as before, on what the last two commits added. --- PyMemoryEditor/app/_widgets.py | 7 +++---- PyMemoryEditor/app/cheat_poll_worker.py | 8 ++++---- PyMemoryEditor/app/main_window.py | 18 ++++++------------ PyMemoryEditor/app/pointer_scan_dialog.py | 3 +-- 4 files changed, 14 insertions(+), 22 deletions(-) diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index 9cf241b..5b75001 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -69,10 +69,9 @@ def detach_worker(worker: QThread) -> None: way park them here too, so threads that outlived their owner are all in one place. """ - # Opportunistic sweep: a worker that finished between its caller's - # isRunning() check and the connect below never reaches its reaper, and Qt - # emits `finished` before isFinished() flips — so the check afterwards can - # miss it too. Pruning here keeps those from piling up. + # Sweep: Qt emits `finished` before isFinished() flips, so a worker that + # finished while being detached can miss both its reaper and the check + # below, and would sit here for good. for parked in list(_DETACHED_WORKERS): try: if parked.isFinished(): diff --git a/PyMemoryEditor/app/cheat_poll_worker.py b/PyMemoryEditor/app/cheat_poll_worker.py index 9c0b153..790f916 100644 --- a/PyMemoryEditor/app/cheat_poll_worker.py +++ b/PyMemoryEditor/app/cheat_poll_worker.py @@ -132,7 +132,7 @@ def _drain_pending_writes(self) -> List[Tuple[int, type, int, str]]: failures: List[Tuple[int, type, int, str]] = [] for address, pytype, length, value in pending: if self._stopping(): - break # same reason as _poll_once: the handle is about to go + break # the handle is about to go — see _poll_once try: self._process.write_process_memory(address, pytype, length, value) except Exception as exc: # noqa: BLE001 @@ -213,9 +213,9 @@ def _poll_once( values_by_address = None for address in addresses: - # Checked per entry, not once per tick: every entry is a syscall - # and shutdown() closes the handle shortly after stop(), so a - # write landing on a recycled handle would hit another process. + # Per entry, not per tick: shutdown() closes the handle right + # after stop(), and a write on a recycled one hits another + # process. if self._stopping(): return results frozen_value, is_frozen = freeze_by_addr[(pytype, length, address)] diff --git a/PyMemoryEditor/app/main_window.py b/PyMemoryEditor/app/main_window.py index 2848c82..cee207b 100644 --- a/PyMemoryEditor/app/main_window.py +++ b/PyMemoryEditor/app/main_window.py @@ -884,13 +884,9 @@ def _change_process(self) -> None: old_cheat.shutdown() except Exception: pass - # The old poller has been stopped, so the handle can go. Accepted - # window: a worker that blew its join is *detached*, not stopped, and - # can still be inside a syscall here (a recycled HANDLE on Windows, a - # deallocated Mach port on macOS). The cheat poller is the one that - # *writes*, which is why it now checks the stop flag per entry rather - # than per tick — blowing the join takes a single syscall hanging for a - # second, not a slow table. + # Accepted window: a worker that blew its join is detached, not + # stopped, so it can still be inside a syscall here. The cheat poller + # writes, which is why it checks the stop flag per entry. try: old_process.close() except Exception: @@ -913,11 +909,9 @@ def closeEvent(self, event: QCloseEvent) -> None: # dialog mid-teardown. self._heartbeat.stop() - # Dialogs next, and *before* _shutdown_worker: it pumps the event - # loop, and a dialog still polling during that pump can reach - # _handle_failed and pop a modal mid-teardown — the same reason the - # heartbeat is stopped first. Closing them also joins their threads, - # which this window's destruction would otherwise take down with it. + # Before _shutdown_worker for the same reason as the heartbeat: it + # pumps the loop, and a dialog still polling during that pump can pop a + # modal mid-teardown. Closing them also joins their threads. for attr in ( "_memory_map", "_threads_dialog", diff --git a/PyMemoryEditor/app/pointer_scan_dialog.py b/PyMemoryEditor/app/pointer_scan_dialog.py index ea6c58c..a51a6aa 100644 --- a/PyMemoryEditor/app/pointer_scan_dialog.py +++ b/PyMemoryEditor/app/pointer_scan_dialog.py @@ -721,8 +721,7 @@ def _append_rows(self, rows: List[_ResultRow]) -> None: ) def _on_error(self, message: str) -> None: - # A metacall queued before _teardown disconnected still gets delivered, - # and a modal over a dismissed dialog is the hazard this PR is about. + # A metacall queued before _teardown disconnected still arrives. if self._is_dismissed(): return QMessageBox.critical(self, "Pointer Scan", message) From 567e97ff14f2e536579f775871bb99983062a45f Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 18 Aug 2026 19:48:21 -0300 Subject: [PATCH 15/15] refactor(app): drop a sweep guarding an unverified race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registry sweep was justified by Qt emitting `finished` before `isFinished()` flips. Measured on this build: 200 out of 200 samples had `isFinished()` already true inside the handler, so the claim doesn't hold and the sweep guarded nothing reproducible. The fallback that does have a demonstrated case — a worker that finished before the reaper was connected — stays. --- PyMemoryEditor/app/_widgets.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/PyMemoryEditor/app/_widgets.py b/PyMemoryEditor/app/_widgets.py index 5b75001..8234fd0 100644 --- a/PyMemoryEditor/app/_widgets.py +++ b/PyMemoryEditor/app/_widgets.py @@ -69,19 +69,11 @@ def detach_worker(worker: QThread) -> None: way park them here too, so threads that outlived their owner are all in one place. """ - # Sweep: Qt emits `finished` before isFinished() flips, so a worker that - # finished while being detached can miss both its reaper and the check - # below, and would sit here for good. - for parked in list(_DETACHED_WORKERS): - try: - if parked.isFinished(): - _reap_detached_worker(parked) - except RuntimeError: # already deleted - _DETACHED_WORKERS.remove(parked) - worker.setParent(None) _DETACHED_WORKERS.append(worker) worker.finished.connect(lambda: _reap_detached_worker(worker)) + # A worker that finished before this connect already emitted, so the reaper + # would never run and it would sit here for good. if worker.isFinished(): _reap_detached_worker(worker)