feat: gracefully shutdown command when shutdown signal is received #493

Robert-maker1994 · 2025-02-03T17:10:16Z

Checklist

[Yes] I have read Contributing > Pull requests

Related issue

Fixes: #469
Currently, the dev CLI panics when a user hits CTRL+C.

Overview

Implemented an receiver shutdown to handle async jobs, plus wait till all the process have stopped.
I have created a mpsc::channel to send a shutdown signal to the main watch function, this singal triggers a graceful shutdown of both the rust server and the SSR bundle build.

I have used tokio::spawn ensureing a shutdown process that is executed asynchronously, with explicitly stopping the running server and the ssr bundle.

crates/tuono/src/watch.rs

Valerioageno · 2025-02-04T14:56:59Z

crates/tuono/src/watch.rs

+                build_ssr_bundle.stop().await;
+                run_server.stop().await;
+            });
+            action.quit_gracefully(Signal::Interrupt, std::time::Duration::from_secs(9999));


Doesn't this automatically stops all the jobs?

https://docs.rs/watchexec/latest/watchexec/action/struct.ActionHandler.html#method.quit_gracefully

crates/tuono/src/watch.rs

Valerioageno · 2025-02-05T17:32:46Z

crates/tuono/src/watch.rs

@@ -116,6 +114,7 @@ pub async fn watch() -> Result<()> {
    // watch the current directory
    wx.config.pathset(["./src"]);

-    let _ = wx.main().await.into_diagnostic()?;
+    let _ = wx.main().await.into_diagnostic();


Why we remove here the error propagation?

Valerioageno · 2025-02-16T19:51:27Z

crates/tuono/src/watch.rs

+            rust_server.delete();
+            build_ssr_bundle.delete();
+            action.quit_gracefully(Signal::Interrupt, std::time::Duration::from_secs(9999));
+            eprintln!("Tuono gracefully shutting down...");


There is no need for the users to be informed about this process

Suggested change

eprintln!("Tuono gracefully shutting down...");

Valerioageno · 2025-02-16T19:52:10Z

@jacobhq are you interested in taking a look?

jacobhq · 2025-02-16T20:35:23Z

@jacobhq are you interested in taking a look?

Yeah if needed. Is there any specific thing that we're stuck on?

Valerioageno · 2025-02-17T07:40:50Z

Nono the PR is ready. I was asking if you are interested in reviewing it

jacobhq · 2025-02-17T07:54:04Z

crates/tuono/src/watch.rs

-            action.quit();
+            rust_server.delete();
+            build_ssr_bundle.delete();
+            action.quit_gracefully(Signal::Interrupt, std::time::Duration::from_secs(9999));


I think we should have a shorter delay maybe like ~30s.

marcalexiei

I tested the patch on this branch on my MacBook Air (M3) and the issue is still present.
I always get tokio-runtime-worker panic error when pressing CTRL + C

Attached screenshot

I'm using examples/tuono-tutorial folder as test case.
Am I missing something 😅?

Robert-maker1994 · 2025-02-17T19:26:43Z

@marcalexiei Interesting, it should work out of the box. We may be getting different results, as I'm running Windows 11. However, if the problem persists, could someone investigate it further?

I would handle this use case as I did previously by adding a select! block to catch the Ctrl+C signal.

jacobhq · 2025-02-17T19:48:07Z

I will take a look, as I am also on Win11 :)

marcalexiei · 2025-02-17T19:56:55Z

Maybe this can be related: https://docs.rs/tokio/latest/tokio/signal/fn.ctrl_c.html#caveats

jacobhq · 2025-02-17T20:19:09Z

I still get the panic too:

tuono\examples\tuono-app on  feat/gracefull-shutdown is 📦 v0.0.1 via ⬢ v22.12.0 via 🦀 v1.83.0 
❯ ../../target/debug/tuono.exe dev

  ⚡ Tuono v0.17.7
  Ready at: http://localhost:3000

thread 'tokio-runtime-worker' panicked at C:\Users\jacob\.cargo\registry\src\index.crates.io-6f17d22bba15001f\watchexec-supervisor-3.0.0\src\job\task.rs:54:17:
all branches are disabled and there is no else branch
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

There are some open tasks from code review. Please address those (they may resolve this issue, since we may be stopping stuff twice at the moment), and then see if you can fix this.

Happy to help if needed, but please finish code review first :)

Robert-maker1994 · 2025-02-17T22:24:47Z

I merged with main and now all of the changed files on main are now on this PR now :( Just an FYI - not to sure how to get rid of them all.

@jacobhq I did add the resolve the comments about the duration to 30 seconds on it killed the remaining job that I missed. However, I'm still receiving the panic from the open jobs.

jacobhq

You're in a tricky place.

I would suggest reverting cc78c84 with git reset HEAD~1 --hard (undoes last commit and deletes changes). Then use update your fork by pulling tuono-labs/tuono main (or Github UI). Let me know if you need help with that.

git merge main when on this branch to bring in changes. Please don't rebase, as you'll rewrite history, and there's a lot of code review that would be messed up. Resolve the conflicts and commit.

After you have merged main, address code review in a separate commit.

I did add the resolve the comments about the duration to 30 seconds on it killed the remaining job that I missed. However, I'm still receiving the panic from the open jobs.

I will take a look tomorrow. Probably priority is to merge main at the moment.

Robert-maker1994 · 2025-02-18T06:43:22Z

@jacobhq Thanks for you're help, that was a right PITA! Everything is in order now, if you view all the commits and not individually, you shall see my changes.

jacobhq

Thanks for this. Can confirm this works on my machine.

jacobhq · 2025-02-18T11:17:33Z

crates/tuono/src/watch.rs

-            action.quit();
+            rust_server.delete();
+            build_ssr_bundle.delete();
+            build_rust_src.delete();


Using .delete() does work here although I don't think it gracefully shuts down the jobs. @Valerioageno WDYT?

jacobhq · 2025-02-18T11:17:51Z

crates/tuono/src/watch.rs

@@ -21,7 +20,6 @@ const DEV_SSR_BIN_SRC: &str = "node_modules\\.bin\\tuono-dev-ssr.cmd";
 const DEV_WATCH_BIN_SRC: &str = "node_modules/.bin/tuono-dev-watch";
 #[cfg(not(target_os = "windows"))]
 const DEV_SSR_BIN_SRC: &str = "node_modules/.bin/tuono-dev-ssr";
-


Please add this new line back in

Robert-maker1994 added 2 commits February 3, 2025 15:33

feat: gracefully shouting down dev command

28bb5d3

Merge branch 'main' into feat/gracefull-shutdown

c183d2a

Robert-maker1994 requested a review from Valerioageno as a code owner February 3, 2025 17:10

github-actions bot added the rust Requires rust knowledge label Feb 3, 2025

remove unused dependancy

8af1906

Robert-maker1994 changed the title ~~Feat(crates\tuono\src\watch): Shutdown gracefully.~~ feat: Shutdown gracefully. Feb 3, 2025

Robert-maker1994 marked this pull request as draft February 4, 2025 11:33

format

774a479

Robert-maker1994 marked this pull request as ready for review February 4, 2025 12:14

Valerioageno reviewed Feb 4, 2025

View reviewed changes

Valerioageno assigned Robert-maker1994 Feb 4, 2025

fix: remove channel and quit_gracefully

e46c198

Valerioageno reviewed Feb 5, 2025

View reviewed changes

marcalexiei changed the title ~~feat: Shutdown gracefully.~~ feat: gracefully shutdown command when interrupt signal is received Feb 5, 2025

Robert-maker1994 added 3 commits February 5, 2025 18:53

adding the error condition to into diagnostic

df8d0a5

adding select again

89d1e96

format

cd79bd2

marcalexiei changed the title ~~feat: gracefully shutdown command when interrupt signal is received~~ feat: gracefully shutdown command when shutdown signal is received Feb 5, 2025

Robert-maker1994 added 3 commits February 11, 2025 21:15

handling async jobs on shutdown

3a53451

Merge branch 'main' into feat/gracefull-shutdown

a02b36e

formatting

a322361

Valerioageno reviewed Feb 16, 2025

View reviewed changes

Removed log in quitting gracefully

677e345

Robert-maker1994 closed this Feb 16, 2025

Robert-maker1994 reopened this Feb 16, 2025

jacobhq reviewed Feb 17, 2025

View reviewed changes

marcalexiei reviewed Feb 17, 2025

View reviewed changes

Adding build_rust_src to be deleted on ctrl-c and duration of 30

cc78c84

github-actions bot added typescript Requires typescript knowledge CI/CD repo maintenance linting, fs organization, that sort of stuff labels Feb 17, 2025

This comment was marked as duplicate.

Sign in to view

Robert-maker1994 closed this Feb 17, 2025

Robert-maker1994 reopened this Feb 17, 2025

jacobhq suggested changes Feb 17, 2025

View reviewed changes

Robert-maker1994 and others added 4 commits February 18, 2025 07:29

after reset

d25db97

after reset

aeaa5b5

merging main

91c343e

Merge branch 'tuono-labs:main' into feat/gracefull-shutdown

1cf5b08

github-actions bot removed typescript Requires typescript knowledge CI/CD repo maintenance linting, fs organization, that sort of stuff labels Feb 18, 2025

Robert-maker1994 added 2 commits February 18, 2025 07:39

reset and merge main

477bfd2

removing unstable imports

2841ad1

jacobhq reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: gracefully shutdown command when shutdown signal is received #493

feat: gracefully shutdown command when shutdown signal is received #493

Robert-maker1994 commented Feb 3, 2025 •

edited

Loading

Valerioageno Feb 4, 2025

Valerioageno Feb 5, 2025

Valerioageno Feb 16, 2025

Valerioageno commented Feb 16, 2025

jacobhq commented Feb 16, 2025

Valerioageno commented Feb 17, 2025

jacobhq Feb 17, 2025

marcalexiei left a comment •

edited

Loading

Robert-maker1994 commented Feb 17, 2025 •

edited

Loading

jacobhq commented Feb 17, 2025 •

edited

Loading

marcalexiei commented Feb 17, 2025

jacobhq commented Feb 17, 2025

This comment was marked as duplicate.

Robert-maker1994 commented Feb 17, 2025 •

edited

Loading

jacobhq left a comment •

edited

Loading

Robert-maker1994 commented Feb 18, 2025

jacobhq left a comment

jacobhq Feb 18, 2025

jacobhq Feb 18, 2025

feat: gracefully shutdown command when shutdown signal is received #493

Are you sure you want to change the base?

feat: gracefully shutdown command when shutdown signal is received #493

Conversation

Robert-maker1994 commented Feb 3, 2025 • edited Loading

Checklist

Related issue

Overview

Valerioageno Feb 4, 2025

Choose a reason for hiding this comment

Valerioageno Feb 5, 2025

Choose a reason for hiding this comment

Valerioageno Feb 16, 2025

Choose a reason for hiding this comment

Valerioageno commented Feb 16, 2025

jacobhq commented Feb 16, 2025

Valerioageno commented Feb 17, 2025

jacobhq Feb 17, 2025

Choose a reason for hiding this comment

marcalexiei left a comment • edited Loading

Choose a reason for hiding this comment

Robert-maker1994 commented Feb 17, 2025 • edited Loading

jacobhq commented Feb 17, 2025 • edited Loading

marcalexiei commented Feb 17, 2025

jacobhq commented Feb 17, 2025

This comment was marked as duplicate.

Robert-maker1994 commented Feb 17, 2025 • edited Loading

jacobhq left a comment • edited Loading

Choose a reason for hiding this comment

Robert-maker1994 commented Feb 18, 2025

jacobhq left a comment

Choose a reason for hiding this comment

jacobhq Feb 18, 2025

Choose a reason for hiding this comment

jacobhq Feb 18, 2025

Choose a reason for hiding this comment

Robert-maker1994 commented Feb 3, 2025 •

edited

Loading

marcalexiei left a comment •

edited

Loading

Robert-maker1994 commented Feb 17, 2025 •

edited

Loading

jacobhq commented Feb 17, 2025 •

edited

Loading

Robert-maker1994 commented Feb 17, 2025 •

edited

Loading

jacobhq left a comment •

edited

Loading