nandi/jolt-native · e86c31c

⑂ e86c31c

Fill the slot, and do not wedge on the way out

Two things a real call found.

An :image was never drawn larger than its own pixels, which is right for a
picture in a message — enlarging a screenshot to fill a column makes it
worse — and wrong for a video tile, which is a slot whose size the layout
decided. A camera sending 480 wide into a 900-point slot sat at 480 in the
middle of it, so a window dragged wider changed nothing anyone could see.
:upscale says which kind this is.

And leaving a call hung the app. joltmoq_stop waited for the media task
while holding the slot lock, and the task's status callback locks that same
slot from a worker thread: the task could not finish, and the thread that
would have released the lock was waiting for the task to finish. The UI
thread is the one that hangs, which is every thread the caller has.

The session now comes out from under the lock before anything waits on it,
and the teardown is spawned rather than waited for — a network round trip
is not something to freeze a window for. It still unpublishes properly; an
abandoned broadcast leaves peers subscribed to someone who looks present
and is silent. There is a test that deadlocks if either goes back.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

nandi committed 2026-08-30T17:54:03-07:00 Browse files

e86c31c parent: 42dabb0

modified crates/jolt-moq/src/lib.rs +85 -31 Viewed

@@ -72,10 +72,6 @@ struct Slot {
72	72	/// The frame most recently handed out. Held so that the pixel pointer the
73	73	/// caller was given stays alive until the next poll replaces it.
74	74	frame: Option<(String, RgbaVideoFrame)>,
75		- /// A tokio runtime of our own: the caller has no reactor to lend us. Made
76		- /// once and kept, since building one per call would tear worker threads
77		- /// down and up again on every join.
78		- runtime: Option<tokio::runtime::Runtime>,
79	75	}
80	76
81	77	#[derive(Clone)]
@@ -90,6 +86,26 @@ fn session_slot() -> &'static Mutex<Slot> {
90	86	SLOT.get_or_init(\|\| Mutex::new(Slot::default()))
91	87	}
92	88
	89	+/// A tokio runtime of our own: the caller has no reactor to lend us.
	90	+///
	91	+/// Deliberately not in the slot. Tearing a call down needs the runtime and
	92	+/// must not be holding the slot lock while it does — see [`joltmoq_stop`].
	93	+/// Made once and kept, since building one per call would tear worker threads
	94	+/// down and up again on every join.
	95	+fn runtime() -> Option<&'static tokio::runtime::Runtime> {
	96	+ static RUNTIME: OnceLock<Option<tokio::runtime::Runtime>> = OnceLock::new();
	97	+ RUNTIME
	98	+ .get_or_init(\|\| {
	99	+ tokio::runtime::Builder::new_multi_thread()
	100	+ .enable_all()
	101	+ .thread_name("joltmoq")
	102	+ .build()
	103	+ .map_err(\|e\| log::error!("joltmoq: no runtime: {e}"))
	104	+ .ok()
	105	+ })
	106	+ .as_ref()
	107	+}
	108	+
93	109	/// Run `f` against the one session, catching panics on the way out.
94	110	///
95	111	/// `fallback` answers both a panic and a poisoned lock, and is deliberately
@@ -158,6 +174,10 @@ pub unsafe extern "C" fn joltmoq_start(
158	174	let mic_id = preference(borrowed(mic_id));
159	175	let speaker_id = preference(borrowed(speaker_id));
160	176
	177	+ let Some(runtime) = runtime() else {
	178	+ return 0;
	179	+ };
	180	+
161	181	with_slot(0, \|slot\| {
162	182	if slot.session.is_some() {
163	183	log::warn!("joltmoq: a call is already live; stop it first");
@@ -168,21 +188,6 @@ pub unsafe extern "C" fn joltmoq_start(
168	188	return 0;
169	189	};
170	190
171		- let runtime = match slot.runtime.take() {
172		- Some(runtime) => runtime,
173		- None => match tokio::runtime::Builder::new_multi_thread()
174		- .enable_all()
175		- .thread_name("joltmoq")
176		- .build()
177		- {
178		- Ok(runtime) => runtime,
179		- Err(e) => {
180		- log::error!("joltmoq: no runtime: {e}");
181		- return 0;
182		- }
183		- },
184		- };
185		-
186	191	let config = AvMediaConfig {
187	192	sfu_url: url,
188	193	session_id,
@@ -220,7 +225,6 @@ pub unsafe extern "C" fn joltmoq_start(
220	225
221	226	slot.video = Some(session.video.clone());
222	227	slot.session = Some(session);
223		- slot.runtime = Some(runtime);
224	228	slot.seen.clear();
225	229	slot.frame = None;
226	230	1
@@ -234,20 +238,40 @@ pub unsafe extern "C" fn joltmoq_start(
234	238	/// it, so they see someone present and hear silence.
235	239	#[no_mangle]
236	240	pub extern "C" fn joltmoq_stop() {
237		- with_slot((), \|slot\| {
238		- let Some(mut session) = slot.session.take() else {
239		- return;
240		- };
241		- match slot.runtime.as_ref() {
242		- Some(runtime) => {
243		- runtime.block_on(session.stop_and_wait(std::time::Duration::from_secs(2)))
244		- }
245		- None => session.stop(),
246		- }
	241	+ // Take the session out from under the lock and let the lock go before
	242	+ // anything waits on it.
	243	+ //
	244	+ // Waiting while holding it deadlocks, and did: the status callback locks
	245	+ // this same slot from a tokio worker, so a task that still had an update
	246	+ // to deliver could not finish, while the thread that would have released
	247	+ // the lock was waiting for exactly that task to finish. The caller's UI
	248	+ // thread is the one that hangs, which is every thread it has.
	249	+ let session = with_slot(None, \|slot\| {
247	250	slot.video = None;
248	251	slot.seen.clear();
249	252	slot.frame = None;
250		- })
	253	+ slot.session.take()
	254	+ });
	255	+ let Some(mut session) = session else {
	256	+ return;
	257	+ };
	258	+
	259	+ // Not waited for either. Tearing MoQ down is a network round trip, and the
	260	+ // press that asked for it was on the thread that paints — half a second of
	261	+ // frozen window is not the answer to "leave". The teardown still runs, and
	262	+ // still unpublishes properly: an abandoned broadcast lingers on the SFU and
	263	+ // peers subscribe to it, seeing someone present who is silent.
	264	+ match runtime() {
	265	+ Some(runtime) => {
	266	+ runtime.spawn(async move {
	267	+ session
	268	+ .stop_and_wait(std::time::Duration::from_secs(2))
	269	+ .await;
	270	+ });
	271	+ }
	272	+ // No runtime means no call ever started; nothing to unwind gracefully.
	273	+ None => session.stop(),
	274	+ }
251	275	}
252	276
253	277	/// 1 while a call is live on this side.
@@ -776,6 +800,36 @@ mod tests {
776	800	assert_eq!(joltmoq_is_live(), 0);
777	801	}
778	802
	803	+ #[test]
	804	+ fn stopping_does_not_hold_the_lock_a_status_callback_needs() {
	805	+ // The deadlock this guards: `stop` used to wait for the media task
	806	+ // while holding the slot, and the task's status callback locks the
	807	+ // slot to deliver an update — so neither could finish. Here a thread
	808	+ // takes the slot the way that callback does, while the main thread
	809	+ // stops. If stop waits under the lock, this never returns.
	810	+ use std::sync::mpsc;
	811	+ use std::time::Duration;
	812	+
	813	+ let (tx, rx) = mpsc::channel();
	814	+ std::thread::spawn(move \|\| {
	815	+ for _ in 0..200 {
	816	+ if let Ok(mut slot) = session_slot().lock() {
	817	+ slot.status.push(Status::Ended);
	818	+ slot.status.clear();
	819	+ }
	820	+ std::thread::sleep(Duration::from_millis(1));
	821	+ }
	822	+ let _ = tx.send(());
	823	+ });
	824	+
	825	+ for _ in 0..50 {
	826	+ joltmoq_stop();
	827	+ }
	828	+ assert_eq!(joltmoq_is_live(), 0);
	829	+ rx.recv_timeout(Duration::from_secs(10))
	830	+ .expect("the status thread never got the lock back");
	831	+ }
	832	+
779	833	#[test]
780	834	fn a_frame_poll_yields_each_feed_once_until_it_changes() {
781	835	let video = VideoFrameStore::new();

      /// The frame most recently handed out. Held so that the pixel pointer the
      /// caller was given stays alive until the next poll replaces it.
      frame: Option<(String, RgbaVideoFrame)>,
--    /// A tokio runtime of our own: the caller has no reactor to lend us. Made
--    /// once and kept, since building one per call would tear worker threads
--    /// down and up again on every join.
--    runtime: Option<tokio::runtime::Runtime>,
  }
  #[derive(Clone)]
      SLOT.get_or_init(|| Mutex::new(Slot::default()))
  }
++/// A tokio runtime of our own: the caller has no reactor to lend us.
++///
++/// Deliberately *not* in the slot. Tearing a call down needs the runtime and
++/// must not be holding the slot lock while it does — see [`joltmoq_stop`].
++/// Made once and kept, since building one per call would tear worker threads
++/// down and up again on every join.
++fn runtime() -> Option<&'static tokio::runtime::Runtime> {
++    static RUNTIME: OnceLock<Option<tokio::runtime::Runtime>> = OnceLock::new();
++    RUNTIME
++        .get_or_init(|| {
++            tokio::runtime::Builder::new_multi_thread()
++                .enable_all()
++                .thread_name("joltmoq")
++                .build()
++                .map_err(|e| log::error!("joltmoq: no runtime: {e}"))
++                .ok()
++        })
++        .as_ref()
++}
++
  /// Run `f` against the one session, catching panics on the way out.
  ///
  /// `fallback` answers both a panic and a poisoned lock, and is deliberately
      let mic_id = preference(borrowed(mic_id));
      let speaker_id = preference(borrowed(speaker_id));
++    let Some(runtime) = runtime() else {
++        return 0;
++    };
++
      with_slot(0, |slot| {
          if slot.session.is_some() {
              log::warn!("joltmoq: a call is already live; stop it first");
              return 0;
          };
--        let runtime = match slot.runtime.take() {
--            Some(runtime) => runtime,
--            None => match tokio::runtime::Builder::new_multi_thread()
--                .enable_all()
--                .thread_name("joltmoq")
--                .build()
--            {
--                Ok(runtime) => runtime,
--                Err(e) => {
--                    log::error!("joltmoq: no runtime: {e}");
--                    return 0;
--                }
--            },
--        };
--
          let config = AvMediaConfig {
              sfu_url: url,
              session_id,
          slot.video = Some(session.video.clone());
          slot.session = Some(session);
--        slot.runtime = Some(runtime);
          slot.seen.clear();
          slot.frame = None;
  /// it, so they see someone present and hear silence.
  #[no_mangle]
  pub extern "C" fn joltmoq_stop() {
--    with_slot((), |slot| {
++    // Take the session out from under the lock and let the lock go *before*
--        let Some(mut session) = slot.session.take() else {
++    // anything waits on it.
--            return;
++    //
--        };
++    // Waiting while holding it deadlocks, and did: the status callback locks
--        match slot.runtime.as_ref() {
++    // this same slot from a tokio worker, so a task that still had an update
--            Some(runtime) => {
++    // to deliver could not finish, while the thread that would have released
--                runtime.block_on(session.stop_and_wait(std::time::Duration::from_secs(2)))
++    // the lock was waiting for exactly that task to finish. The caller's UI
--            }
++    // thread is the one that hangs, which is every thread it has.
--            None => session.stop(),
++    let session = with_slot(None, |slot| {
--        }
          slot.video = None;
          slot.seen.clear();
          slot.frame = None;
--    })
++        slot.session.take()
++    });
++    let Some(mut session) = session else {
++        return;
++    };
++
++    // Not waited for either. Tearing MoQ down is a network round trip, and the
++    // press that asked for it was on the thread that paints — half a second of
++    // frozen window is not the answer to "leave". The teardown still runs, and
++    // still unpublishes properly: an abandoned broadcast lingers on the SFU and
++    // peers subscribe to it, seeing someone present who is silent.
++    match runtime() {
++        Some(runtime) => {
++            runtime.spawn(async move {
++                session
++                    .stop_and_wait(std::time::Duration::from_secs(2))
++                    .await;
++            });
++        }
++        // No runtime means no call ever started; nothing to unwind gracefully.
++        None => session.stop(),
++    }
  }
  /// 1 while a call is live on this side.
          assert_eq!(joltmoq_is_live(), 0);
      }
++    #[test]
++    fn stopping_does_not_hold_the_lock_a_status_callback_needs() {
++        // The deadlock this guards: `stop` used to wait for the media task
++        // while holding the slot, and the task's status callback locks the
++        // slot to deliver an update — so neither could finish. Here a thread
++        // takes the slot the way that callback does, while the main thread
++        // stops. If stop waits under the lock, this never returns.
++        use std::sync::mpsc;
++        use std::time::Duration;
++
++        let (tx, rx) = mpsc::channel();
++        std::thread::spawn(move || {
++            for _ in 0..200 {
++                if let Ok(mut slot) = session_slot().lock() {
++                    slot.status.push(Status::Ended);
++                    slot.status.clear();
++                }
++                std::thread::sleep(Duration::from_millis(1));
++            }
++            let _ = tx.send(());
++        });
++
++        for _ in 0..50 {
++            joltmoq_stop();
++        }
++        assert_eq!(joltmoq_is_live(), 0);
++        rx.recv_timeout(Duration::from_secs(10))
++            .expect("the status thread never got the lock back");
++    }
++
      #[test]
      fn a_frame_poll_yields_each_feed_once_until_it_changes() {
          let video = VideoFrameStore::new();

modified crates/jolt-vidya/include/vidya_tree.h +7 -0 Viewed

@@ -57,6 +57,13 @@ extern "C" {
57	57	* status label, live (bool)
58	58	*/
59	59
	60	+/* An `:image` is bounded by `max-width` / `max-height` and, by default, never
	61	+ * drawn larger than its own pixels — enlarging a screenshot to fill a column
	62	+ * makes it worse. `upscale` says otherwise, for a picture whose size the
	63	+ * layout decided rather than the file: a video tile is a slot, and a camera
	64	+ * sending 480 wide into a 900-point slot should fill it.
	65	+ */
	66	+
60	67	/* The window node, created on first use. Mount everything under it. */
61	68	VIDYA_API int vidya_tree_root(void);
62	69

   *   status        label, live (bool)
   */
++/* An `:image` is bounded by `max-width` / `max-height` and, by default, never
++ * drawn larger than its own pixels — enlarging a screenshot to fill a column
++ * makes it worse. `upscale` says otherwise, for a picture whose size the
++ * layout decided rather than the file: a video tile is a slot, and a camera
++ * sending 480 wide into a 900-point slot should fill it.
++ */
++
  /* The window node, created on first use. Mount everything under it. */
  VIDYA_API int vidya_tree_root(void);

modified crates/jolt-vidya/src/tree.rs +17 -1 Viewed

@@ -1299,7 +1299,23 @@ impl Tree {
1299	1299	} else {
1300	1300	ui.available_width()
1301	1301	};
1302		- let scale = (avail / size.x).min(max_height / size.y).min(1.0);
	1302	+ // A picture in a message is never enlarged past its own
	1303	+ // pixels: blowing up a screenshot to fill a column makes it
	1304	+ // worse, and the reader can open it if they want it bigger.
	1305	+ //
	1306	+ // `:upscale` says this one is different. A video tile is a
	1307	+ // slot whose size the layout decided — how many people are
	1308	+ // in the call, how big the window is — and a camera sending
	1309	+ // 480 wide into a 900-point slot should fill it, the way every
	1310	+ // other video surface does. Left off, the picture would sit at
	1311	+ // its own size in the middle of a space reserved for it and
	1312	+ // the layout would look broken.
	1313	+ let scale = (avail / size.x).min(max_height / size.y);
	1314	+ let scale = if props.bool("upscale", false) {
	1315	+ scale
	1316	+ } else {
	1317	+ scale.min(1.0)
	1318	+ };
1303	1319	// Clickable whether or not the caller listens: the tree does
1304	1320	// not know which nodes have handlers, and an unheard event
1305	1321	// costs a queue slot.

                  } else {
                      ui.available_width()
                  };
--                let scale = (avail / size.x).min(max_height / size.y).min(1.0);
++                // A picture in a message is never enlarged past its own
++                // pixels: blowing up a screenshot to fill a column makes it
++                // worse, and the reader can open it if they want it bigger.
++                //
++                // `:upscale` says this one is different. A video tile is a
++                // *slot* whose size the layout decided — how many people are
++                // in the call, how big the window is — and a camera sending
++                // 480 wide into a 900-point slot should fill it, the way every
++                // other video surface does. Left off, the picture would sit at
++                // its own size in the middle of a space reserved for it and
++                // the layout would look broken.
++                let scale = (avail / size.x).min(max_height / size.y);
++                let scale = if props.bool("upscale", false) {
++                    scale
++                } else {
++                    scale.min(1.0)
++                };
                  // Clickable whether or not the caller listens: the tree does
                  // not know which nodes have handlers, and an unheard event
                  // costs a queue slot.