[feat] odemis-park-mirror: allow to cancel when using the GUI mode#3380
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds cancellation functionality to the GUI mode of the odemis-park-mirror utility. Previously, only the CLI mode supported cancellation via Ctrl+C. The implementation adds a Cancel button to the progress dialog and refactors the code to support cancellation from both CLI and GUI modes.
Changes:
- Added cancellation support in GUI mode via a Cancel button in the progress dialog
- Refactored park/mirror access logic to support passing a
future_holderfor external cancellation - Updated exception handling to use
CancelledErrorconsistently across CLI and GUI modes
| # Close dialog on main thread | ||
| wx.CallAfter(dlg.close_dialog) | ||
| # Close dialog on main thread, if not already closed by Cancel | ||
| if dlg.IsShown(): |
There was a problem hiding this comment.
There's a potential issue with checking dlg.IsShown() in a worker thread. The IsShown() method is a wxPython GUI method that should only be called from the main GUI thread. Calling it from the worker thread could cause undefined behavior or crashes. Consider using a flag or removing this check entirely, as wx.CallAfter is safe to call even if the dialog has been destroyed.
| self.gauge = wx.Gauge(panel, range=100, size=(300, 25)) | ||
| sizer.Add(self.gauge, 0, wx.ALL | wx.EXPAND, 10) | ||
|
|
||
| # Cancel button |
There was a problem hiding this comment.
The condition check on line 145 if self._future_holder: only checks if the list exists, not if it contains a valid future. This is correct for showing the button (we want to show it whenever a future_holder is passed), but the comment on line 144 could be clearer. Consider updating the comment to say "Cancel button (only shown if future_holder is provided)".
| # Cancel button | |
| # Cancel button (only shown if future_holder is provided) |
In the command line mode, it's possible to cancel with Ctrl+C. In the GUI mode, it wasn't possible. => Add a "cancel" button.
pieleric
force-pushed
the
feat-odemis-park-mirror-allow-to-cancel-when-using-the-gui-mode
branch
from
402c63e to
2e63d1d
Compare
📝 WalkthroughWalkthroughThis PR refactors the mirror parking utility to introduce thread-safe cancellation support. It introduces Sequence DiagramsequenceDiagram
participant Main as Main Thread
participant Dialog as ProgressDialog
participant BG as Background Thread
participant ParkState as ParkState (FutureHolder)
participant Mirror as Mirror Component
Main->>ParkState: Create shared state
Main->>Dialog: Create with FutureHolder reference
Main->>BG: Spawn background parking thread
activate BG
Main->>Dialog: Show dialog (blocked)
BG->>BG: Call park(mirror, future_holder)
alt User clicks Cancel
Main->>Dialog: User clicks Cancel button
Dialog->>ParkState: Cancel shared future
BG->>BG: CancelledError raised
BG->>ParkState: Store exception
else Normal operation
BG->>Mirror: Execute parking moves
BG->>ParkState: Store completed future
end
BG->>Main: Thread completes
deactivate BG
Main->>ParkState: Check final state
Main->>Dialog: Close and return exit code
Main->>Main: Handle result (success/cancel/error)
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@util/odemis-park-mirror`:
- Around line 67-91: There’s a tiny race between mirror.reference(...) and
assigning future_holder.future so a cancel can slip past; fix by giving
FutureHolder a thread-safe API (e.g., add methods set_future(self, f: Future)
and cancel(self)) that use a lock and a cancelled flag: implement set_future to
store the future and immediately cancel it if cancelled==True, and implement
cancel() to flip cancelled and cancel any stored future; then in park() replace
direct assignments to future_holder.future with future_holder.set_future(f) and
ensure _on_cancel calls future_holder.cancel().
- Around line 230-233: The background thread is calling dlg.IsShown() (not
wx-thread-safe) and may schedule close_dialog for a dlg that gets Destroy()'d
before the callback runs; instead always post a main-thread callback via
wx.CallAfter (never call IsShown() from the background thread) that performs the
dlg state check and calls close_dialog if still valid, and move the
dlg.Destroy() call so it runs after the worker thread has been joined
(thread.join()) to avoid the race; update references to dlg, close_dialog,
wx.CallAfter, IsShown(), Destroy(), and thread.join() accordingly.
| def park(mirror: model.Actuator, future_holder: Optional[FutureHolder] = None) -> None: | ||
| # Need to park in two moves: first S, then L | ||
| f = mirror.reference({"s"}) | ||
| if future_holder is not None: | ||
| future_holder.future = f | ||
| logging.info("Parking the mirror...") | ||
| try: | ||
| logging.debug("Moving S axis") | ||
| f.result() | ||
| f.result() # Can raise CancelledError if cancelled, or other exceptions if move failed | ||
| except KeyboardInterrupt: | ||
| f.cancel() | ||
| logging.warning("Cancelled parking move") | ||
| raise | ||
| raise CancelledError() | ||
|
|
||
| f = mirror.reference({"l"}) | ||
| if future_holder is not None: | ||
| future_holder.future = f | ||
| try: | ||
| logging.debug("Moving L axis") | ||
| f.result() | ||
| except KeyboardInterrupt: | ||
| f.cancel() | ||
| logging.warning("Cancelled parking move") | ||
| raise | ||
| raise CancelledError() | ||
|
|
There was a problem hiding this comment.
Minor race window between creating the new future and storing it in future_holder.
If the user presses Cancel between mirror.reference({"l"}) (Line 81) and the assignment on Line 83, _on_cancel will cancel the already-completed first future (a no-op), and the second move will proceed uninterrupted. The cancel press is silently lost.
This is a narrow window with a safe fallback (the move just completes), so it's unlikely to matter in practice, but worth being aware of.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/odemis-park-mirror` around lines 67 - 91, There’s a tiny race between
mirror.reference(...) and assigning future_holder.future so a cancel can slip
past; fix by giving FutureHolder a thread-safe API (e.g., add methods
set_future(self, f: Future) and cancel(self)) that use a lock and a cancelled
flag: implement set_future to store the future and immediately cancel it if
cancelled==True, and implement cancel() to flip cancelled and cancel any stored
future; then in park() replace direct assignments to future_holder.future with
future_holder.set_future(f) and ensure _on_cancel calls future_holder.cancel().
| finally: | ||
| # Close dialog on main thread | ||
| wx.CallAfter(dlg.close_dialog) | ||
| # Close dialog on main thread, if not already closed by Cancel | ||
| if dlg.IsShown(): | ||
| wx.CallAfter(dlg.close_dialog) |
There was a problem hiding this comment.
dlg.IsShown() called from a background thread is not wx-thread-safe; also risk of calling close_dialog on a destroyed dialog.
Accessing wx widget state from a non-main thread is undefined behavior in wxPython. Additionally, if the cancel path runs and dlg.Destroy() is called on Line 241 before the wx.CallAfter callback executes, close_dialog would operate on a destroyed window.
A safer pattern is to always post to the main thread and guard inside the callback:
Proposed fix
finally:
- # Close dialog on main thread, if not already closed by Cancel
- if dlg.IsShown():
- wx.CallAfter(dlg.close_dialog)
+ # Close dialog on main thread, if not already closed by Cancel.
+ # Always use CallAfter to avoid accessing wx objects from this thread.
+ def _safe_close():
+ if dlg and dlg.IsShown():
+ dlg.close_dialog()
+ wx.CallAfter(_safe_close)Even with this, there's a residual race with dlg.Destroy() on Line 241. Consider moving dlg.Destroy() after thread.join() to ensure the background thread's CallAfter is posted (and harmlessly skipped) before the dialog is destroyed:
Suggested reordering
# Show modal dialog (blocks until closed by park_thread or Cancel button)
modal_result = dlg.ShowModal()
- dlg.Destroy()
# Wait for thread to complete
thread.join(timeout=2.0)
if thread.is_alive():
logging.warning("Parking thread is still running after dialog closed.")
+
+ dlg.Destroy()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/odemis-park-mirror` around lines 230 - 233, The background thread is
calling dlg.IsShown() (not wx-thread-safe) and may schedule close_dialog for a
dlg that gets Destroy()'d before the callback runs; instead always post a
main-thread callback via wx.CallAfter (never call IsShown() from the background
thread) that performs the dlg state check and calls close_dialog if still valid,
and move the dlg.Destroy() call so it runs after the worker thread has been
joined (thread.join()) to avoid the race; update references to dlg,
close_dialog, wx.CallAfter, IsShown(), Destroy(), and thread.join() accordingly.
1 participant
In the command line mode, it's possible to cancel with Ctrl+C. In the
GUI mode, it wasn't possible.
=> Add a "cancel" button.