Handle ctrl+c to gracefully shutdown the server(s) #1613

Wumpf · 2023-03-20T09:27:04Z

Fixes

Tried to use tokio's ctrl-c detection but ran into issues with it, so used the ctrlc crate we already depend on and got it working nicely with that.

Tested ctrl'c'ing with:

Serving

cargo run -p rerun --features web_viewer -- ../api_demo.rrd --web-viewer
cargo run -p rerun --features web_viewer -- --web-viewer
cargo run -p re_web_viewer_server
cargo run -p api_demo -- --serve
python ./examples/python/api_demo/main.py --serve
python3 -m rerun --web-viewer ../objectron.rrd

Not serving, just to see if something else broke

python ./examples/python/tracking_hf_opencv/main.py
cargo run -p api_demo

Checklist

I have read and agree to Contributor Guide and the Code of Conduct

emilk · 2023-03-20T09:46:32Z

I tested python3 -m rerun --web-viewer ../objectron.rrd, and that too seems to work

crates/re_web_viewer_server/src/main.rs

crates/rerun/src/run.rs

crates/rerun/src/web_viewer.rs

emilk · 2023-03-20T10:00:07Z

crates/rerun/src/web_viewer.rs


        let web_server_join_handle = tokio_rt.spawn(async move {
            // This is the server which the web viewer will talk to:
            let ws_server = re_ws_comms::Server::new(re_ws_comms::DEFAULT_WS_SERVER_PORT)
                .await
                .unwrap();
-            let ws_server_handle = tokio::spawn(ws_server.listen(rerun_rx)); // TODO(emilk): use tokio_rt ?
+            let ws_server_handle = tokio::spawn(ws_server.listen(rerun_rx, shutdown_rx_ws_server)); // TODO(emilk): use tokio_rt ?


Do we really need this channel for the ws server? Doesn't calling self.web_server_join_handle.abort() (as we do in drop) already work?

That's one of the advantages of async tasks in general: they are easy to cancel (just drop their handles).

Hm. I could try ripping out the channel again from both servers and go with abort. Wonder if that is the same really as the shutdown I'm doing now.
I'd suggest not going there and unify as suggested above by using shutdown_txon Drop

dropping a handle does not cancel a task though, one needs to call abort on it explicitly? (only found this old thread where that decision was made, maybe has changed since then tokio-rs/tokio#1830 (comment))
Also do all tasks implement proper abort? The channel setup here follows tokio's recommendation on how to deal with ctrl+c

I thought abort would work on any async task, since it simply stops the future from being polled. I don't understand why tokio would recommend something else.

Where is this recommendation?

https://tokio.rs/tokio/topics/shutdown

let next_frame = tokio::select! { res = self.connection.read_frame() => res?, _ = self.shutdown.recv() => { // If a shutdown signal is received, return from `run`. // This will result in the task terminating. return Ok(()); } };

Maybe this is a bit too much out of context we're dealing with more specific tasks after all. Furthermore, most of this does boil down to an abort I reckon. The reason I ended up here was not exhaustive research and consideration of what we have, but rather seeing that example and going with it, discovering stuff in the process.
That said, I'm actually quite happy with this solution here since it gives us a single super easy entry point for signaling shut down which also makes it easy to work with hyper's with_graceful_shutdown

rerun_py/rerun_sdk/rerun/script_helpers.py

Wumpf · 2023-03-20T13:44:17Z

cargo run -p rerun --features web_viewer -- --web-viewer does not work correctly

Wumpf · 2023-03-20T14:22:00Z

oof this has bigger ramifications.

…l shutdown mechanism Had to move setup_ctrl_c_handler back to `rerun` because of this

Wumpf · 2023-03-20T14:31:48Z

works now. Tested all serve usecases again (added to the list)

Wumpf · 2023-03-20T14:38:17Z

more issues! I didn't try cargo run -p api_demo -- --serve before (which runs into the remaining sleep in clap.rs). Cancelling works but it doesn't serve properly

…n't create a new runtime if there is one already. Gracefully wait on ctrl+c instead of sleeping for `RerunArgs` entrypoint

Wumpf · 2023-03-20T16:08:32Z

that one is passing now as well! Changes got a lot bigger, but I do think it's better - admittedly still learning all these tokio things though; got quite far 😄

…serve-exit

emilk

Looks great 👏👏👏

crates/re_ws_comms/src/server.rs

crates/rerun/src/clap.rs

emilk · 2023-03-21T07:24:29Z

crates/rerun/src/clap.rs

@@ -74,6 +74,18 @@ impl RerunArgs {
        default_enabled: bool,
        run: impl FnOnce(Session) + Send + 'static,
    ) -> anyhow::Result<()> {
+        // Ensure we have a running tokio runtime.


crates/rerun/src/web_viewer.rs

This reverts commit d666c94.

emilk · 2023-03-21T08:17:51Z

Found a problem after merge:

cargo r -p rerun -- ../nyud.rrd causes this:

[2023-03-21T08:15:56Z INFO  rerun::run] Loading "../nyud.rrd"…

thread 'main' panicked at 'Error setting Ctrl-C handler: MultipleHandlers', re_viewer/src/app.rs:131

   7: core::panicking::panic_fmt
             at core/src/panicking.rs:64:14
   8: core::result::unwrap_failed
             at core/src/result.rs:1791:5
   9: core::result::Result<T,E>::expect
             at core/src/result.rs:1070:23
      re_viewer::app::App::from_receiver
             at re_viewer/src/app.rs:126:13
  10: rerun::run::run_impl::{{closure}}::{{closure}}
             at rerun/src/run.rs:357:27
      core::ops::function::FnOnce::call_once{{vtable.shim}}
             at core/src/ops/function.rs:507:5
  11: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at alloc/src/boxed.rs:2000:9

Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting
libc++abi: terminating with uncaught foreign exception


Rerun caught a signal: SIGABRT

Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting

      rerun::crash_handler::install_signal_handler::signal_handler
             at rerun/src/crash_handler.rs:163:25
   3: _OSAtomicTestAndClearBarrier
   4: __pthread_atfork_prepare_handlers
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>

Rerun caught a signal: SIGABRT
Troubleshooting Rerun: https://www.rerun.io/docs/getting-started/troubleshooting

zsh: abort      cargo r -p rerun -- ../nyud.rrd

Reverting in #1632

emilk · 2023-03-21T08:22:02Z

I think the solution to the above problem would be to hand down shutdown_rx to crates/re_viewer/src/app.rs, and have that spawn a task to listen for the shutdown signal

…though I am not a big fan of how tokio is infecting every part of the codebase :/

…1632) This reverts commit d666c94.

…1613)" (#1632)" This reverts commit 93ab88b.

* Revert "Revert "Handle ctrl+c to gracefully shutdown the server(s) (#1613)" (#1632)" This reverts commit 93ab88b. * pass shutdown bool to app instead of installing another ctrl+c handler

Handle ctrl+c to gracefully shutdown the server(s)

6adb7fa

Wumpf added the 🪳 bug Something isn't working label Mar 20, 2023

formatting

bd98911

emilk reviewed Mar 20, 2023

View reviewed changes

Wumpf added 2 commits March 20, 2023 14:38

move setup_ctrl_c_handler to re_web_viewer_server

2efb8d8

comment fix, use channel for server shutdown in RemoteViewerServer

686f274

Wumpf added the do-not-merge Do not merge this PR label Mar 20, 2023

use tokio in re_web_viewer_server/server in order to use same gracefu…

3cd7aef

…l shutdown mechanism Had to move setup_ctrl_c_handler back to `rerun` because of this

Wumpf added 💣 crash crash, deadlock/freeze, do-no-start and removed do-not-merge Do not merge this PR labels Mar 20, 2023

emilk marked this pull request as draft March 20, 2023 15:10

Wumpf added 3 commits March 20, 2023 16:23

don't create tokio runtime if there is already one

e3d2027

move responsibility for alive tokio runtime outside of web_viewer. Do…

078c92d

…n't create a new runtime if there is one already. Gracefully wait on ctrl+c instead of sleeping for `RerunArgs` entrypoint

Fix python serve lacking a tokio runtime

b71d15a

Wumpf marked this pull request as ready for review March 20, 2023 16:07

Wumpf added 2 commits March 20, 2023 17:51

Merge remote-tracking branch 'origin/main' into andreas/fix-crash-on-…

0b6f312

…serve-exit

fix doc test

d295ed9

Wumpf requested a review from emilk March 20, 2023 17:05

Fix warnings

9a15af3

emilk approved these changes Mar 21, 2023

View reviewed changes

emilk added 2 commits March 21, 2023 08:26

improve docstring

236a4f0

don't unwrap, especially in drop

96f7556

emilk merged commit d666c94 into main Mar 21, 2023

emilk deleted the andreas/fix-crash-on-serve-exit branch March 21, 2023 07:40

emilk added a commit that referenced this pull request Mar 21, 2023

Revert "Handle ctrl+c to gracefully shutdown the server(s) (#1613)"

3cfc05f

This reverts commit d666c94.

emilk mentioned this pull request Mar 21, 2023

Revert "Handle ctrl+c to gracefully shutdown the server(s)" #1632

Merged

emilk added a commit that referenced this pull request Mar 21, 2023

Revert "Handle ctrl+c to gracefully shutdown the server(s) (#1613)" (#…

93ab88b

…1632) This reverts commit d666c94.

Wumpf added a commit that referenced this pull request Mar 21, 2023

Revert "Revert "Handle ctrl+c to gracefully shutdown the server(s) (#…

13a5a52

…1613)" (#1632)" This reverts commit 93ab88b.

Wumpf mentioned this pull request Mar 21, 2023

Fix crash on serve exit, second attempt #1633

Merged

1 task

emilk mentioned this pull request Mar 27, 2023

Add a script that generates a changelog from recent PRs and their labels #1718

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle ctrl+c to gracefully shutdown the server(s) #1613

Handle ctrl+c to gracefully shutdown the server(s) #1613

Wumpf commented Mar 20, 2023 •

edited

Loading

emilk commented Mar 20, 2023

emilk Mar 20, 2023 •

edited

Loading

Wumpf Mar 20, 2023

Wumpf Mar 20, 2023 •

edited

Loading

emilk Mar 20, 2023 •

edited

Loading

Wumpf Mar 20, 2023 •

edited

Loading

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

emilk left a comment

emilk Mar 21, 2023

emilk commented Mar 21, 2023

emilk commented Mar 21, 2023

Handle ctrl+c to gracefully shutdown the server(s) #1613

Handle ctrl+c to gracefully shutdown the server(s) #1613

Conversation

Wumpf commented Mar 20, 2023 • edited Loading

Checklist

emilk commented Mar 20, 2023

emilk Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Wumpf Mar 20, 2023

Choose a reason for hiding this comment

Wumpf Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

emilk Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Wumpf Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

Wumpf commented Mar 20, 2023

emilk left a comment

Choose a reason for hiding this comment

emilk Mar 21, 2023

Choose a reason for hiding this comment

emilk commented Mar 21, 2023

emilk commented Mar 21, 2023

Wumpf commented Mar 20, 2023 •

edited

Loading

emilk Mar 20, 2023 •

edited

Loading

Wumpf Mar 20, 2023 •

edited

Loading

emilk Mar 20, 2023 •

edited

Loading

Wumpf Mar 20, 2023 •

edited

Loading