Driver: Fix scheduler crash after task closure

A removed audio task could still have one or more driver messages left in its queue, leading to a crash when the id->mixer lookup failed. This removes an unwrap which is invalid under these assumptions and includes an extra cleanup measure for message forwarders under the same circumstances.

This was tested using `cargo make ready`.
This commit is contained in:
Kyle Simpson
2023-06-07 21:15:30 +01:00
parent c976d50cc5
commit 77e3916bdc
4 changed files with 43 additions and 19 deletions

View File

@@ -3,6 +3,7 @@ use std::{collections::HashMap, sync::Arc, time::Duration};
use flume::{Receiver, Sender}; use flume::{Receiver, Sender};
use nohash_hasher::{BuildNoHashHasher, IntMap}; use nohash_hasher::{BuildNoHashHasher, IntMap};
use tokio::time::{Instant as TokInstant, Interval}; use tokio::time::{Instant as TokInstant, Interval};
use tracing::warn;
use crate::constants::*; use crate::constants::*;
@@ -71,14 +72,14 @@ impl Idle {
biased; biased;
msg = self.rx.recv_async() => match msg { msg = self.rx.recv_async() => match msg {
Ok(SchedulerMessage::NewMixer(rx, ic, cfg)) => { Ok(SchedulerMessage::NewMixer(rx, ic, cfg)) => {
let mixer = ParkedMixer::new(rx, ic, cfg); let mut mixer = ParkedMixer::new(rx, ic, cfg);
let id = self.next_id.incr(); let id = self.next_id.incr();
mixer.spawn_forwarder(self.tx.clone(), id); mixer.spawn_forwarder(self.tx.clone(), id);
self.tasks.insert(id, mixer); self.tasks.insert(id, mixer);
self.stats.add_idle_mixer(); self.stats.add_idle_mixer();
}, },
Ok(SchedulerMessage::Demote(id, task)) => { Ok(SchedulerMessage::Demote(id, mut task)) => {
task.send_gateway_not_speaking(); task.send_gateway_not_speaking();
task.spawn_forwarder(self.tx.clone(), id); task.spawn_forwarder(self.tx.clone(), id);
@@ -86,15 +87,17 @@ impl Idle {
}, },
Ok(SchedulerMessage::Do(id, mix_msg)) => { Ok(SchedulerMessage::Do(id, mix_msg)) => {
let now_live = mix_msg.is_mixer_now_live(); let now_live = mix_msg.is_mixer_now_live();
let task = self.tasks.get_mut(&id).unwrap(); if let Some(task) = self.tasks.get_mut(&id) {
match task.handle_message(mix_msg) {
match task.handle_message(mix_msg) { Ok(false) if now_live => {
Ok(false) if now_live => { let task = self.tasks.remove(&id).unwrap();
let task = self.tasks.remove(&id).unwrap(); self.schedule_mixer(task, id, None);
self.schedule_mixer(task, id, None); },
}, Ok(false) => {},
Ok(false) => {}, Ok(true) | Err(_) => self.to_cull.push(id),
Ok(true) | Err(_) => self.to_cull.push(id), }
} else {
warn!("Received post-cull message for {id:?}, discarding.");
} }
}, },
Ok(SchedulerMessage::Overspill(worker_id, id, task)) => { Ok(SchedulerMessage::Overspill(worker_id, id, task)) => {
@@ -136,7 +139,9 @@ impl Idle {
} }
for id in self.to_cull.drain(..) { for id in self.to_cull.drain(..) {
self.tasks.remove(&id); if let Some(tx) = self.tasks.remove(&id).and_then(|t| t.cull_handle) {
_ = tx.send_async(()).await;
}
} }
true true
@@ -292,6 +297,7 @@ mod test {
rtp_timestamp: i, rtp_timestamp: i,
park_time: TokInstant::now().into(), park_time: TokInstant::now().into(),
last_cost: None, last_cost: None,
cull_handle: None,
}; };
core.stats.add_idle_mixer(); core.stats.add_idle_mixer();
core.stats.move_mixer_to_live(); core.stats.move_mixer_to_live();

View File

@@ -554,6 +554,7 @@ impl Live {
rtp_timestamp: id_0 as u32, rtp_timestamp: id_0 as u32,
park_time: Instant::now(), park_time: Instant::now(),
last_cost: None, last_cost: None,
cull_handle: None,
}, },
id, id,
Instant::now(), Instant::now(),
@@ -617,6 +618,7 @@ impl Live {
rtp_timestamp, rtp_timestamp,
park_time, park_time,
last_cost: None, last_cost: None,
cull_handle: None,
}, },
) )
}) })

View File

@@ -75,6 +75,8 @@ pub struct ParkedMixer {
/// The last known cost of executing this task, if it had to be moved /// The last known cost of executing this task, if it had to be moved
/// due to a limit on thread resources. /// due to a limit on thread resources.
pub last_cost: Option<Duration>, pub last_cost: Option<Duration>,
/// Handle to any forwarder task, used if this mixer is culled while idle.
pub cull_handle: Option<Sender<()>>,
} }
#[allow(missing_docs)] #[allow(missing_docs)]
@@ -88,6 +90,7 @@ impl ParkedMixer {
rtp_timestamp: random::<u32>(), rtp_timestamp: random::<u32>(),
park_time: Instant::now(), park_time: Instant::now(),
last_cost: None, last_cost: None,
cull_handle: None,
} }
} }
@@ -95,14 +98,28 @@ impl ParkedMixer {
/// ///
/// Any requests which would cause this mixer to become live will terminate /// Any requests which would cause this mixer to become live will terminate
/// this task. /// this task.
pub fn spawn_forwarder(&self, tx: Sender<SchedulerMessage>, id: TaskId) { pub fn spawn_forwarder(&mut self, tx: Sender<SchedulerMessage>, id: TaskId) {
let (kill_tx, kill_rx) = flume::bounded(1);
self.cull_handle = Some(kill_tx);
let remote_rx = self.mixer.mix_rx.clone(); let remote_rx = self.mixer.mix_rx.clone();
tokio::spawn(async move { tokio::spawn(async move {
while let Ok(msg) = remote_rx.recv_async().await { loop {
let exit = msg.is_mixer_now_live(); tokio::select! {
let dead = tx.send_async(SchedulerMessage::Do(id, msg)).await.is_err(); biased;
if exit || dead { _ = kill_rx.recv_async() => break,
break; msg = remote_rx.recv_async() => {
let exit = if let Ok(msg) = msg {
let remove_self = msg.is_mixer_now_live();
tx.send_async(SchedulerMessage::Do(id, msg)).await.is_err() || remove_self
} else {
true
};
if exit {
break;
}
}
} }
} }
}); });

View File

@@ -13,7 +13,6 @@ use reqwest::{
header::{HeaderMap, ACCEPT_RANGES, CONTENT_LENGTH, CONTENT_TYPE, RANGE, RETRY_AFTER}, header::{HeaderMap, ACCEPT_RANGES, CONTENT_LENGTH, CONTENT_TYPE, RANGE, RETRY_AFTER},
Client, Client,
}; };
use std::fmt::format;
use std::{ use std::{
io::{Error as IoError, ErrorKind as IoErrorKind, Result as IoResult, SeekFrom}, io::{Error as IoError, ErrorKind as IoErrorKind, Result as IoResult, SeekFrom},
pin::Pin, pin::Pin,