Skip to content

Conversation

@hongquanli
Copy link
Contributor

Summary

  • Use os._exit() instead of sys.exit() to prevent segmentation fault when closing the application
  • PyQt5's C++ destructor order conflicts with Python's garbage collector during normal shutdown

Test plan

  • Run python3 main_hcs.py --simulation and close the application
  • Verify no segmentation fault occurs on exit

🤖 Generated with Claude Code

Use os._exit() instead of sys.exit() to avoid a segmentation fault
when closing the application. PyQt5's C++ destructor order conflicts
with Python's garbage collector during the normal shutdown sequence,
causing a segfault.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR attempts to fix a segmentation fault on application exit by replacing sys.exit() with os._exit(). While this prevents the segfault caused by PyQt5's C++ destructor conflicts with Python's garbage collector, it introduces a critical problem by bypassing all cleanup handlers and the GUI's closeEvent method that performs essential hardware shutdown operations.

Key Changes

  • Changed exit mechanism from sys.exit(app.exec_()) to os._exit(app.exec_())
  • Added explanatory comment about PyQt5 destructor ordering issues

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 261 to 264
# Use os._exit() to avoid segfault during Python's shutdown sequence.
# PyQt5's C++ destructor order conflicts with Python's garbage collector
# during normal shutdown (sys.exit), causing a segfault.
os._exit(app.exec_())
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using os._exit() bypasses all cleanup handlers, which will prevent critical shutdown operations from executing:

  1. The app.aboutToQuit.connect(control_server.stop) handler on line 90 won't be called, potentially leaving the MCP control server socket open
  2. The GUI's closeEvent method (lines 1700-1766 in gui_hcs.py) won't be called, which performs essential hardware cleanup including:
    • Stopping camera streaming and closing the camera
    • Retracting the Z stage to safe position
    • Turning off hardware components (filters, lasers, microcontroller, etc.)
    • Closing fluidics and other resources

os._exit() immediately terminates the process without calling destructors or cleanup handlers, which can leave hardware in unsafe states and resources unclosed.

A better approach would be to ensure proper cleanup order by explicitly calling cleanup methods before the normal exit, or by fixing the specific destructor ordering issue in the PyQt5 code. Alternatively, consider using sys.exit() but ensuring all Qt objects are properly deleted before Python's garbage collection phase (e.g., by setting objects to None or using deleteLater()).

Suggested change
# Use os._exit() to avoid segfault during Python's shutdown sequence.
# PyQt5's C++ destructor order conflicts with Python's garbage collector
# during normal shutdown (sys.exit), causing a segfault.
os._exit(app.exec_())
# Run the Qt event loop and perform a normal shutdown so that all
# Qt and application cleanup handlers (aboutToQuit, closeEvent, etc.)
# are executed before the process exits.
exit_code = app.exec_()
sys.exit(exit_code)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Investigated and improved comment in 07eab6a.

The concern about skipped cleanup is based on a misunderstanding of the Qt event loop. Critical cleanup handlers ARE called before os._exit():

  1. closeEvent() - Runs when the user closes the window, which happens before app.exec_() returns. All hardware cleanup (camera, stage retraction, microcontroller shutdown) completes here.

  2. aboutToQuit - Qt emits this signal before app.exec_() returns. Confirmed working: logs show "Microscope control server stopped" on exit.

Why os._exit() is necessary:

  • Tested sip.setdestroyonexit(False) + sys.exit() - still segfaults
  • The PyQt5/Python GC conflict happens after app.exec_() returns, during Python's shutdown phase
  • os._exit() skips only Python's post-Qt cleanup (atexit handlers, __del__ methods), which are redundant since closeEvent already handles hardware cleanup

The updated comment now documents this behavior clearly.

Expand the comment explaining why os._exit() is used to address
reviewer concerns. The comment now clarifies that critical cleanup
handlers (closeEvent, aboutToQuit) ARE called before os._exit()
because they run before app.exec_() returns.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 264 to 269
# Note: This does NOT skip critical cleanup because:
# - closeEvent() runs when the window closes (before app.exec_() returns)
# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
os._exit(app.exec_())
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aboutToQuit signal connected to control_server.stop() (line 90) may not be sufficient when using os._exit(). While the comment claims aboutToQuit fires before app.exec_() returns, os._exit() will still bypass normal thread cleanup. The ConsoleThread (line 77) and MicroscopeControlServer's daemon thread (line 268 in microscope_control_server.py) will be abruptly terminated without graceful shutdown, potentially leaving sockets open or losing buffered data.

Suggested change
# Note: This does NOT skip critical cleanup because:
# - closeEvent() runs when the window closes (before app.exec_() returns)
# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
os._exit(app.exec_())
# Note: This does NOT skip critical GUI/hardware cleanup because:
# - closeEvent() runs when the window closes (before app.exec_() returns)
# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
exit_code = app.exec_()
# Best-effort shutdown of background threads before forcing process exit.
try:
if USE_TERMINAL_CONSOLE and "console_thread" in locals():
try:
if hasattr(console_thread, "stop"):
console_thread.stop()
if hasattr(console_thread, "join"):
console_thread.join(timeout=5)
except Exception:
log.exception("Error while shutting down console thread")
if ENABLE_MCP_SERVER_SUPPORT and "control_server" in locals():
try:
if hasattr(control_server, "stop"):
control_server.stop()
if hasattr(control_server, "join"):
control_server.join(timeout=5)
except Exception:
log.exception("Error while shutting down microscope control server")
except Exception:
# Ensure that any unexpected error during cleanup does not prevent exit.
log.exception("Unexpected error during shutdown cleanup")
os._exit(exit_code)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Partially addressed.

The aboutToQuit signal (line 90) already handles control_server.stop() before app.exec_() returns - this is confirmed by logs showing "Microscope control server stopped" on exit.

For ConsoleThread, it's a daemon thread that will be terminated when the main process exits. Adding explicit shutdown is possible but adds complexity for minimal benefit since console history loss is acceptable (addressed in comment 4 reply).

The critical hardware cleanup (camera, stage, microcontroller) happens in closeEvent() which completes before os._exit().

Comment on lines 261 to 269
# Use os._exit() to prevent segfault during Python's shutdown sequence.
# PyQt5's C++ destructor order conflicts with Python's garbage collector.
#
# Note: This does NOT skip critical cleanup because:
# - closeEvent() runs when the window closes (before app.exec_() returns)
# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
os._exit(app.exec_())
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment claims that closeEvent runs before app.exec_() returns and handles all hardware cleanup. However, this doesn't address the underlying segfault issue. A better approach would be to explicitly delete Qt objects in proper order before calling sys.exit(), or to investigate and fix the root cause of the segfault (e.g., circular references, improper parent-child relationships, or double-deletion of C++ objects). Using os._exit() masks the problem rather than solving it.

Suggested change
# Use os._exit() to prevent segfault during Python's shutdown sequence.
# PyQt5's C++ destructor order conflicts with Python's garbage collector.
#
# Note: This does NOT skip critical cleanup because:
# - closeEvent() runs when the window closes (before app.exec_() returns)
# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
os._exit(app.exec_())
# Run the Qt event loop and exit with its return code using normal Python shutdown.
# All critical hardware cleanup should be handled in closeEvent() / aboutToQuit
# handlers before app.exec_() returns.
exit_code = app.exec_()
sys.exit(exit_code)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Investigated - root cause fix not feasible.

We tested sip.setdestroyonexit(False) which tells PyQt5 not to destroy C++ objects during Python exit - this is the standard fix for this class of issues. It still segfaults.

The segfault occurs deep in PyQt5/Qt's C++ destructor ordering during Python's garbage collection phase. Properly fixing this would require:

  1. Identifying which specific Qt objects have problematic destructor ordering
  2. Manually controlling deletion order for potentially dozens of Qt widgets
  3. Risk of introducing memory leaks or new crashes

Given that all critical cleanup (hardware, logging) happens before os._exit(), and the alternatives have been tested, os._exit() is the pragmatic solution used by many PyQt5 applications with this issue.

# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
os._exit(app.exec_())
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation states that file logging will not flush properly with os._exit(). Log handlers use buffering, and os._exit() bypasses Python's cleanup which includes flushing file handles. This could result in loss of final log messages. Since this is a hardware control application where logging is critical for debugging issues, losing the last few log entries on exit could hide important information about shutdown problems.

Suggested change
os._exit(app.exec_())
exit_code = app.exec_()
# Ensure all logging handlers (including file handlers) are flushed and closed
# before bypassing Python's normal shutdown sequence with os._exit().
logging.shutdown()
os._exit(exit_code)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Fixed in 254920b.

Added logging.shutdown() before os._exit():

exit_code = app.exec_()
logging.shutdown()  # Flush log handlers before os._exit() bypasses Python cleanup
os._exit(exit_code)

This ensures all log handlers (including file handlers) are properly flushed before the process exits.

Comment on lines 261 to 269
# Use os._exit() to prevent segfault during Python's shutdown sequence.
# PyQt5's C++ destructor order conflicts with Python's garbage collector.
#
# Note: This does NOT skip critical cleanup because:
# - closeEvent() runs when the window closes (before app.exec_() returns)
# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
os._exit(app.exec_())
Copy link

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using os._exit() bypasses Python's normal cleanup, which will prevent atexit handlers from running. The ConsoleThread uses readline's history saving in an atexit handler (line 192 in console.py), so the console command history will not be saved when the application exits. Consider using sys.exit() with proper signal handling or ensuring Qt objects are properly destroyed before exit instead.

Suggested change
# Use os._exit() to prevent segfault during Python's shutdown sequence.
# PyQt5's C++ destructor order conflicts with Python's garbage collector.
#
# Note: This does NOT skip critical cleanup because:
# - closeEvent() runs when the window closes (before app.exec_() returns)
# - aboutToQuit signal fires before app.exec_() returns
# All hardware cleanup (camera, stage, microcontroller) happens in closeEvent,
# which completes before os._exit() is called.
os._exit(app.exec_())
# Run the Qt event loop and capture the exit code.
exit_code = app.exec_()
# Ensure Qt objects are cleaned up before allowing Python to shut down normally.
try:
win.close()
except Exception:
pass
try:
app.quit()
except Exception:
pass
# Use sys.exit so that Python's normal shutdown (including atexit handlers,
# such as console history saving) is executed.
sys.exit(exit_code)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Acknowledged - acceptable tradeoff.

The suggested approach (using sys.exit() with manual cleanup) still segfaults. The crash occurs during Python's garbage collector phase, not during Qt cleanup. We tested sip.setdestroyonexit(False) and other approaches - the only reliable fix is os._exit().

Tradeoff analysis:

  • Console history is nice-to-have, not critical functionality
  • Console thread is optional (behind USE_TERMINAL_CONSOLE flag)
  • Segfault on every exit is a much worse UX than losing command history

Potential future fix if this becomes a priority: manually call readline.write_history_file() before os._exit() in main_hcs.py. This would preserve history without needing atexit handlers. Creating as a follow-up issue.

Ensures log handlers are flushed before os._exit() bypasses Python's
normal cleanup sequence. This prevents loss of final log messages
which are critical for debugging shutdown issues.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@hongquanli hongquanli merged commit 785ed6e into master Jan 3, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants