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

Simpler SIGINT handling #2198

Merged
merged 9 commits into from
May 24, 2023
Merged

Simpler SIGINT handling #2198

merged 9 commits into from
May 24, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented May 24, 2023

What

python -m rerun needs to shutdown on SIGINT. Python catches SIGINT though, and waits for the GIL to shut down. But if the viewer is running, we never return to Python. So instead we install our own SIGINT handler in Rust and just call std::process::exit on SIGINT.

Testing

Check that Ctrl-C works in all these situations:

  • cargo rerun
    • Linux
    • Mac
    • Windows
  • cargo rerun foo.rrd
    • Linux
    • Mac
    • Windows
  • cargo run -p rerun-cli --features web_viewer -- --web-viewer
    • Linux
    • Mac
    • Windows
  • cargo run -p api_demo -- --serve
    • Linux
    • Mac
    • Windows
  • just py-build && python -m rerun
    • Linux
    • Mac
    • Windows
  • just py-build -F web_viewer && ./examples/python/api_demo/main.py --serve
    • Linux
    • Mac
    • Windows
  • just py-build --features web_viewer && python -m rerun --web-viewer
    • Linux
    • Mac
    • Windows

Checklist

PR Build Summary: https://build.rerun.io/pr/2198

@emilk emilk added the 🚜 refactor Change the code, not the functionality label May 24, 2023
@emilk emilk mentioned this pull request May 24, 2023
14 tasks
@emilk emilk marked this pull request as ready for review May 24, 2023 13:22
@jprochazk
Copy link
Member

jprochazk commented May 24, 2023

Had a lot of trouble setting everything up on Windows. It boils down to a lot of stuff assuming bash exists and symlinks work.

  • setup scripts don't work on Windows
  • winget does not have a binaryen package
  • re_web_viewer_server is configured to get its files through a symlink (/crates/re_web_viewer_server/web_viewer -> /web_viewer) (I think?) and that didn't transfer over properly on Windows. I had to manually edit the paths to the web_viewer files in order to get it to build.
  • just py-build uses bash, which doesn't work on Windows. Had to trigger the build by calling maturin manually

But ctrl-c works in all scenarios there 🙂.

@emilk
Copy link
Member Author

emilk commented May 24, 2023

Awesome - can you please create a tracking issue for all Windows problems?

crates/rerun/src/clap.rs Outdated Show resolved Hide resolved
@jprochazk
Copy link
Member

Awesome - can you please create a tracking issue for all Windows problems?

Done: #2200

emilk and others added 2 commits May 24, 2023 17:34
Co-authored-by: Jan Procházka <1665677+jprochazk@users.noreply.github.com>
@emilk emilk merged commit 165de62 into main May 24, 2023
@emilk emilk deleted the emilk/simpler-sigint-handling branch May 24, 2023 16:01
emilk added a commit that referenced this pull request May 25, 2023
* Call exit(42) on SIGINT when running `python -m rerun`

* Make shutdown_rx optional for re_web_viewer_server

* Remove SIGINT from crash_handler

* Remove shutdown from re_ws_comms::server

* Remove our complex ctrl-c shutdown handling

* cargo fmt

* Fix doctest

* fix typo

Co-authored-by: Jan Procházka <1665677+jprochazk@users.noreply.github.com>

* Use u64::MAX

---------

Co-authored-by: Jan Procházka <1665677+jprochazk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants