Skip to content

Conversation

@exrok
Copy link

@exrok exrok commented Dec 20, 2025

This bug was found when alsactl was repeatedly failing due to a missing device causing a number of alsactl processes to be running.

This PR uses tokio's kill_on_drop and an added the process handle to a field in the device. I have not checked the rest of the code base for similar issues but at least after this PR alsa is fixed. :)

https://docs.rs/tokio/latest/tokio/process/struct.Command.html#method.kill_on_drop

This bug was found when alsactl was repeatedly failing due to a missing
device causing a number of alsactl processes to be running.

This PR uses tokio's kill_on_drop and an added the process handle to a field
in the device. I have not checked the rest of the code base for similar
issues but at least after this PR alsa is fixed. :)
@bim9262
Copy link
Collaborator

bim9262 commented Dec 24, 2025

Why not just keep the process in the struct Device and get the stdout in wait_for_update?

Like this
diff --git a/src/blocks/sound/alsa.rs b/src/blocks/sound/alsa.rs
index efd2f18dc..4fff9e788 100644
--- a/src/blocks/sound/alsa.rs
+++ b/src/blocks/sound/alsa.rs
@@ -1,6 +1,6 @@
 use std::cmp::{max, min};
 use std::process::Stdio;
-use tokio::process::{ChildStdout, Command};
+use tokio::process::Command;
 
 use super::super::prelude::*;
 use super::SoundDevice;
@@ -11,33 +11,23 @@ pub(super) struct Device {
     natural_mapping: bool,
     volume: u32,
     muted: bool,
-    monitor: ChildStdout,
-    #[expect(
-        dead_code,
-        reason = "Only used in drop to trigger kill and reap of child process"
-    )]
     process: tokio::process::Child,
 }
 
 impl Device {
     pub(super) fn new(name: String, device: String, natural_mapping: bool) -> Result<Self> {
-        let mut process = Command::new("alsactl")
-            .arg("monitor")
-            .kill_on_drop(true)
-            .stdout(Stdio::piped())
-            .spawn()
-            .error("Failed to start alsactl monitor")?;
         Ok(Device {
             name,
             device,
             natural_mapping,
             volume: 0,
             muted: false,
-            monitor: process
-                .stdout
-                .take()
-                .error("Failed to pipe alsactl monitor output")?,
-            process,
+            process: Command::new("alsactl")
+                .arg("monitor")
+                .kill_on_drop(true)
+                .stdout(Stdio::piped())
+                .spawn()
+                .error("Failed to start alsactl monitor")?,
         })
     }
 }
@@ -147,7 +137,10 @@ impl SoundDevice for Device {
 
     async fn wait_for_update(&mut self) -> Result<()> {
         let mut buf = [0u8; 1024];
-        self.monitor
+        self.process
+            .stdout
+            .as_mut()
+            .error("Failed to get stdout of alsactl monitor")?
             .read(&mut buf)
             .await
             .error("Failed to read stdbuf output")?;

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