Kill watchers on app close. #7

0xbrayo · 2024-05-09T22:39:26Z

This took longer than I wished it did. Learnt so much about how rust handles concurrency. I experimented with using channels but that would not work because Rust does not allow for multiple receivers to the same channel. I came up with this solution. Does not use a condition variable though. It could be improved I'll revisit it later when I'm better at rust concurrency.

🚀 This description was created by Ellipsis for commit `2d69626`

Summary:

This PR ensures that all watcher processes are terminated when the application is closed by introducing a new alive flag in ManagerState and checking this flag in the watcher's loop.

Key points:

Add alive flag to ManagerState to track if the app is closing.
Modify start_watcher in manager.rs to terminate watchers if alive is false.
Set alive to false in on_tray_event in main.rs when 'quit' is clicked.
Minor formatting changes in build.rs.

Generated with ❤️ by ellipsis.dev

ellipsis-dev

❌ Changes requested. Reviewed everything up to 2d69626 in 2 minutes and 52 seconds

More details

Looked at 167 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_YhM6VmBEyVxbZXt4

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

⌛ 10 days left in your free trial, upgrade for $20/seat/month or contact us.

ellipsis-dev · 2024-05-09T22:42:30Z

src-tauri/src/main.rs

+                let mut manager_state = manager_state.lock().unwrap();
+                manager_state.alive = false;
+                drop(manager_state);
+                sleep(Duration::from_millis(500)); //TODO: revisit this


Using sleep to delay application exit can lead to a poor user experience. Consider using a more responsive approach to ensure all processes are terminated before exiting, such as checking the status of child processes or using event-driven signaling.

I have a few ideas for getting rid of this... Having a variable that tracks number of running watchers. It's a bit tricky to implement as I have drop the mutex so that it's acquired by the start_watcher threads, Then reacquire it and wait till the count is zero before proceeding. Will try it tomorrow!

0xbrayo · 2024-05-10T14:32:38Z

Handles like a dream... I'm so impressed how it came together!

ErikBjare · 2024-05-10T15:58:55Z

src-tauri/src/manager.rs

 }

 impl ManagerState {
    fn new() -> ManagerState {
        ManagerState {
            watchers_running: HashMap::new(),
+            watcher_count: 0,
+            alive: true,


~~Unused~~, not quite true, but I don't think it should be needed.

They are later used. watcher_count keeps track of number of running watchers and alive whether the parent process is ready to exit. I will refactor to discard watcher_count

ErikBjare · 2024-05-10T15:59:28Z

src-tauri/src/manager.rs

 }

 impl ManagerState {
    fn new() -> ManagerState {
        ManagerState {
            watchers_running: HashMap::new(),
+            watcher_count: 0,


Can be computed by counting the number of true in the watchers_running. You could write it as a helper function on ManagerState.

ErikBjare · 2024-05-10T16:02:02Z

src-tauri/src/manager.rs

+                        name: name.to_string(),
+                        output: std::process::Output {
+                            status,
+                            stdout: Vec::new(),


This discards the stdout/stderr from the process, which is a regression.

ErikBjare · 2024-05-10T16:03:57Z

src-tauri/src/manager.rs

-            output,
-        })
-        .unwrap();
+        loop {


This loop doesn't loop clean to me, what are you looping over? When does the looping end?

This loops for the entirety of the program, checks whether the child process is still alive. If it is not sends the output to handle (there was a regression here due to me fighting the borrow checker) and breaks from the loop. The other arm of the match is executed every loop the process is alive. checks if the variable alive is still true. If not kills the process and exits the loop.

ErikBjare · 2024-05-10T16:05:15Z

src-tauri/src/manager.rs

+                    let mut state = state.lock().unwrap();
+                    if !state.alive {
+                        println!("Killing {name}");
+                        child.as_mut().unwrap().kill().unwrap();


That's a lot of unwraps. These need to be handled.

ErikBjare · 2024-05-10T16:07:12Z

src-tauri/src/manager.rs

    thread::spawn(move || {
        // Start the child process
        let path = name;
        let args = ["--testing", "--port", "5699"];
-        let child = Command::new(path)
+        let mut child = Command::new(path)


Why does this need to be mut?

This was me fighting with the borrow checker. It wouldn't work without mut

ErikBjare · 2024-05-10T16:45:01Z

src-tauri/src/manager.rs

-        // Wait for the child to exit
-        let output = child
-            .unwrap()
-            .wait_with_output()
-            .expect("failed to wait on child");
-
-        // Send the process output to the manager
-        tx.send(WatcherMessage::Stopped {
-            name: name.to_string(),
-            output,
-        })


Just an idea: maybe we could keep this, since it's pretty clean, and instead return the process PID in WatcherMessage::Started to be stored in ManagerState, which is then stopped by sending a SIGTERM to the PID from the main thread?

Might remove the need for polling the state, and makes the start_watcher not need the state parameter.

This would greatly simplify everything. Let me get started on it and see!

On the off chance, the process exits and it's not reported to start_manager sending a SIGTERM could potentially kill a random process. "The chances are near zero", Oppenheimer reference. I think the reporting is near realtime but there could be a chance? I still don't think it's an issue really just a random thought.

0xbrayo · 2024-05-10T17:58:10Z

Done. Your way is so much cleaner.

0xbrayo · 2024-05-10T18:08:39Z

Libc is not natively supported in windows it seems.

0xbrayo · 2024-05-10T20:21:25Z

It builds successfully. I haven't tested it on windows yet! Hopefully it works.

src-tauri/src/manager.rs

ErikBjare

This is indeed a lot cleaner, nice!

* chore: cargo fmt * chore: Update Cargo.lock and Cargo.toml with nix and winapi * refactor: Update manager.rs to send SIGTERM to watchers on app exit * chore: bug fix * feat: bug fix

chore: cargo fmt

bcf9626

ellipsis-dev bot reviewed May 9, 2024

View reviewed changes

ErikBjare reviewed May 10, 2024

View reviewed changes

0xbrayo force-pushed the watchers branch from 3e68ae1 to ce8b3d1 Compare May 10, 2024 17:57

0xbrayo force-pushed the watchers branch from ce8b3d1 to 7e82a06 Compare May 10, 2024 18:31

0xbrayo added 2 commits May 10, 2024 22:28

chore: Update Cargo.lock and Cargo.toml with nix and winapi

fdd54bb

refactor: Update manager.rs to send SIGTERM to watchers on app exit

5516505

0xbrayo force-pushed the watchers branch from 7e82a06 to 5516505 Compare May 10, 2024 19:30

0xbrayo added 2 commits May 10, 2024 22:47

chore: bug fix

a1952a1

feat: bug fix

4950967

ErikBjare reviewed May 11, 2024

View reviewed changes

src-tauri/src/manager.rs Show resolved Hide resolved

ErikBjare approved these changes May 11, 2024

View reviewed changes

ErikBjare merged commit 51ab72b into ActivityWatch:master May 11, 2024
3 checks passed

0xbrayo deleted the watchers branch May 13, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kill watchers on app close. #7

Kill watchers on app close. #7

0xbrayo commented May 9, 2024 •

edited by ellipsis-dev bot

Loading

ellipsis-dev bot left a comment

ellipsis-dev bot May 9, 2024

0xbrayo May 9, 2024

0xbrayo commented May 10, 2024

ErikBjare May 10, 2024 •

edited

Loading

0xbrayo May 10, 2024

ErikBjare May 10, 2024

ErikBjare May 10, 2024

ErikBjare May 10, 2024

0xbrayo May 10, 2024

ErikBjare May 10, 2024

0xbrayo May 10, 2024

ErikBjare May 10, 2024

0xbrayo May 10, 2024

ErikBjare May 10, 2024 •

edited

Loading

0xbrayo May 10, 2024

0xbrayo May 10, 2024 •

edited

Loading

0xbrayo commented May 10, 2024

0xbrayo commented May 10, 2024

0xbrayo commented May 10, 2024

ErikBjare left a comment

Kill watchers on app close. #7

Kill watchers on app close. #7

Conversation

0xbrayo commented May 9, 2024 • edited by ellipsis-dev bot Loading

Summary:

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xbrayo commented May 10, 2024

ErikBjare May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikBjare May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xbrayo May 10, 2024 • edited Loading

Choose a reason for hiding this comment

0xbrayo commented May 10, 2024

0xbrayo commented May 10, 2024

0xbrayo commented May 10, 2024

ErikBjare left a comment

Choose a reason for hiding this comment

0xbrayo commented May 9, 2024 •

edited by ellipsis-dev bot

Loading

ErikBjare May 10, 2024 •

edited

Loading

ErikBjare May 10, 2024 •

edited

Loading

0xbrayo May 10, 2024 •

edited

Loading