Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(serve): Add graceful Ctrl+C handling with exit message #1238

Open
wants to merge 4 commits into
base: canary
Choose a base branch
from

Conversation

revidious
Copy link
Contributor

@revidious revidious commented Dec 12, 2024

Graceful Ctrl+C Handling

Implements reliable Ctrl+C handling for BAML's Python interface with a clean shutdown message. This fixes issues with signal handling across the Python/Rust boundary and improves user experience when stopping BAML processes.

Changes

  • Added custom signal handling using the ctrlc crate
  • Implemented clean "Shutting Down BAML..." message on interrupt
  • Bypassed Python's signal handling to ensure reliable behavior
  • Added proper exit code (130) following Unix conventions
  • Added detailed documentation explaining the implementation

Testing

  • Tested Python CLI server start/stop
  • Manual Ctrl+C testing in various scenarios
  • Verified clean shutdown message display
  • Tested cross-platform signal handling behavior

Technical Details

  • Uses ctrlc crate for reliable cross-platform signal handling
  • Implements channel-based communication between signal handler and main thread
  • Follows Unix conventions for interrupt handling (exit code 130)
  • Properly documented implementation rationale and technical decisions

Notes

  • While this is a workaround, it provides reliable interrupt handling without requiring major architectural changes
  • The implementation bypasses Python's signal handling entirely to avoid conflicts
  • Future improvements could involve deeper integration with BAML's architecture

Important

Enhances BAML's Python interface with reliable Ctrl+C handling and clean shutdown message using the ctrlc crate.

  • Behavior:
    • Implements custom signal handling to bypass Python's signal handlers
    • Displays "Shutting Down BAML..." message on interrupt
    • Ensures proper process termination with standard exit codes
  • Implementation:
    • Uses ctrlc crate for cross-platform signal handling
    • Implements channel-based communication for safe signal handling
    • Follows Unix conventions with exit code 130 for SIGINT

This description was created by Ellipsis for the latest commit. It will automatically update as commits are pushed.

Copy link

vercel bot commented Dec 12, 2024

@revidious is attempting to deploy a commit to the Gloo Team on Vercel.

A member of the Team first needs to authorize it.

@revidious revidious force-pushed the feat/graceful-ctrl-c branch from 4534bfc to 2035b8f Compare December 12, 2024 23:18
@sxlijin
Copy link
Collaborator

sxlijin commented Dec 13, 2024

Started a discussion thread in https://discord.com/channels/1119368998161752075/1316923453814734959 - I have some questions about how this plays with pyo3 and different architectures (it sounds like we handle this fine in N-API world?)

Thanks for digging into this!

This implements a custom signal handling mechanism using the ctrlc crate to: 1. Bypass Python's signal handling 2. Provide consistent behavior across platforms 3. Ensure graceful shutdown with proper exit codes

While this is a workaround, it provides reliable interrupt handling without requiring major architectural changes to BAML's signal handling.
@revidious revidious force-pushed the feat/graceful-ctrl-c branch from 2035b8f to d1952d6 Compare December 14, 2024 19:11
@revidious revidious requested a review from sxlijin December 14, 2024 21:32
Copy link
Collaborator

@sxlijin sxlijin left a comment

Choose a reason for hiding this comment

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

This is clever! Nice work with this

We could use a oneshot channel for the signal but I really like this - on the one hand spawning an additional thread feels overkill, but it's also a neat way to avoid wiring in Python::check_signals everywhere

Copy link

vercel bot commented Dec 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 6:06pm

@sxlijin
Copy link
Collaborator

sxlijin commented Dec 18, 2024

Sorry for the delay! I was in a GitHub beta for a new UI and couldn't figure out how to trigger workflows on your PR (maintainers need to explicitly approve PRs from new contributors)

@sxlijin
Copy link
Collaborator

sxlijin commented Dec 18, 2024

Hmm - looks like I can't push to your branch to update Cargo.lock. Can you commit that change?

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