fix(ios): skip AudioDeviceModule JS round-trips when no handler is registered#91
Merged
Conversation
hiroshihorie
changed the title
There was a problem hiding this comment.
Pull request overview
This PR builds on the bounded-wait fix (#90) by eliminating JS round-trips for RTCAudioDeviceModule engine delegate hooks when no JS handler is registered, further reducing the deadlock surface area on iOS.
Changes:
- Track per-hook handler “active” state in JS and push state changes to native.
- Expose iOS native methods to update per-hook active flags on
AudioDeviceModuleObserver. - Skip sending events / waiting on semaphores in
AudioDeviceModuleObserverwhen the corresponding hook is inactive.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/AudioDeviceModuleEvents.ts | Sends “handler active” state changes to native when handlers are set/cleared. |
| ios/RCTWebRTC/WebRTCModule+RTCAudioDeviceModule.m | Adds exported native methods to set per-hook active flags on the observer. |
| ios/RCTWebRTC/AudioDeviceModuleObserver.m | Adds isActive gating to skip JS round-trips for inactive hooks. |
| ios/RCTWebRTC/AudioDeviceModuleObserver.h | Declares atomic per-hook active flags used by the observer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+217
to
+220
| // Notify native about active state change to avoid unnecessary JS round trips | ||
| if (wasActive !== isActive && WebRTCModule) { | ||
| WebRTCModule.audioDeviceModuleSetEngineCreatedActive(isActive); | ||
| } |
Comment on lines
+233
to
+235
| if (wasActive !== isActive && WebRTCModule) { | ||
| WebRTCModule.audioDeviceModuleSetWillEnableEngineActive(isActive); | ||
| } |
Comment on lines
+248
to
+250
| if (wasActive !== isActive && WebRTCModule) { | ||
| WebRTCModule.audioDeviceModuleSetWillStartEngineActive(isActive); | ||
| } |
Comment on lines
+263
to
+265
| if (wasActive !== isActive && WebRTCModule) { | ||
| WebRTCModule.audioDeviceModuleSetDidStopEngineActive(isActive); | ||
| } |
Comment on lines
+278
to
+280
| if (wasActive !== isActive && WebRTCModule) { | ||
| WebRTCModule.audioDeviceModuleSetDidDisableEngineActive(isActive); | ||
| } |
Comment on lines
+293
to
+295
| if (wasActive !== isActive && WebRTCModule) { | ||
| WebRTCModule.audioDeviceModuleSetWillReleaseEngineActive(isActive); | ||
| } |
Comment on lines
+99
to
+104
| if (!isActive) { | ||
| // No handler registered, proceed immediately without JS round trip. | ||
| // This avoids the deadlock window entirely. | ||
| os_log(ADMObserverLog(), "Skipping JS round-trip for %{public}@ (no handler registered)", eventName); | ||
| return 0; | ||
| } |
hiroshihorie
force-pushed
the
hiroshi/fix-adm-skip-js
branch
from
cefcd2c to
777d706
Compare
davidliu
approved these changes
Jun 17, 2026
Track per-hook handler registration with is-prefixed BOOL active flags that JS pushes to native whenever a handler is set or cleared. When a hook has no registered handler the observer returns 0 immediately without a JS round trip, so the unhandled hooks (engineCreated, willStart, didStop, willRelease in stock config) never enter the blocking wait and cannot contribute to the deadlock. Hooks with a registered handler (willEnable, didDisable in stock config) keep the bounded-wait and request-id safety net from the previous change.
The isXxxActive flags are written on the JS thread when a handler is registered or cleared and read on the native audio thread in the delegate callbacks. Declaring them atomic routes those accesses through synchronized accessors, removing the data race (and the ThreadSanitizer warning) while keeping the lighter accessor cost that suits independent single flags. The multi-field requestId state stays under @synchronized(self) because it needs a true critical section.
The native audioDeviceModuleSetXxxActive setters are iOS/macOS only, but the per-handler push was guarded only by WebRTCModule presence. On Android WebRTCModule is truthy while the method is undefined, so the first setXxxHandler call threw a TypeError. Route all six setters through one pushHandlerActive helper gated by Platform.OS !== 'android' (matching setupListeners) so the Android guard cannot drift across copies. Flagged by Copilot and the re-review.
…ile on setup) The active flags defaulted to NO and were pushed only on a handler set/clear transition, with nothing reconciling native state to JS state. A delegate callback that fired before reconciliation (e.g. an observer recreated while the JS handler singleton persisted) would take the skip path and silently drop the handler's veto, letting the engine proceed when the app wanted it aborted. Default every native flag to YES so the worst case is one harmless bounded round trip absorbed by the timeout, not a dropped veto, and push the current handler state for all six hooks in setupListeners() so a fresh observer is reconciled.
The skip path logged via os_log at default level on the realtime audio worker thread for every inactive hook. With all hooks defaulting to active and reconciled to NO in the common no-handler case, this fired on the latency-sensitive path for every engine lifecycle event. Use os_log_debug so it is not persisted in production logs while staying available when debug logging is enabled. Flagged by Copilot and the re-review.
addListener appends subscriptions without de-duping, so a second registerGlobals() left two live listeners per blocking event and invoked each app handler twice per round. Guard setupListeners with a listenersSetUp flag so repeat calls are no-ops.
…ost veto The active-flag optimization skips the JS round trip when a hook is inactive. The setters published the handler to the JS field before pushing the native active flag, so a delegate callback racing registration could read the flag as still inactive and skip a handler that was already installed, silently dropping its veto. Push the flag active before publishing the handler on activation (and clear the handler before pushing inactive on deactivation) via a shared applyHandlerActive helper. The worst case becomes an extra JS round trip whose listener resolves 0, never a skipped handler.
The idempotency guard returned before the active-flag reconcile, so a second setupListeners() (e.g. a recreated native observer behind a surviving JS singleton) would never re-sync the flags, leaving the native defaults (all active) unreconciled. Guard only the addListener registration; extract the reconcile into reconcileActiveFlags() and run it on every call so the native observer always matches the current handler state.
The entry "waiting for JS response" log was already gated on isActive, but the exit "JS returned" log was not, so the skip path emitted a misleading "JS returned: 0" alongside the internal "Skipping JS round-trip" debug line. Gate the exit log (and willEnable's session category diagnostic) on isActive too, so the per-callback round-trip narration appears only when a JS round trip actually happens.
hiroshihorie
force-pushed
the
hiroshi/fix-adm-skip-js
branch
from
04ca4c6 to
4acbcdc
Compare
Merged
davidliu
added a commit
that referenced
this pull request
Jun 18, 2026
a95d57d fix(ios): skip AudioDeviceModule JS round-trips when no handler is registered (#91) ( Hiroshi Horie 2026-06-18 16:20:29 +0900) c751bd6 ios: bump WebRTC-SDK to 144.7559.10 (#92) ( Hiroshi Horie 2026-06-18 16:11:31 +0900) 196cbb3 fix(ios): bound AudioDeviceModuleObserver JS waits to break the deadlock (#90) ( Hiroshi Horie 2026-06-18 03:49:07 +0900)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds on #90 (the bounded-wait deadlock fix) to remove the JS round-trip entirely for AudioDeviceModule engine hooks that have no registered handler, instead of only bounding the wait. For those hooks this closes the deadlock window rather than capping it at 2 seconds.
Stacked on #90. This PR targets the #90 branch, so it should land after #90 and the diff here is only the delta on top.
Background
#90 bounds each of the six RTCAudioDeviceModule delegate waits to 2 seconds so a stuck JS thread can no longer freeze the app forever. But a hook with no JS handler has nothing to wait for, so blocking it at all is pure risk with no benefit.
Change
Track per-hook handler registration with is-prefixed BOOL active flags. The JS layer pushes a flag to native whenever a handler is set or cleared. When a hook is inactive the observer returns 0 immediately without sending the event or waiting, so the unhandled hooks never enter the blocking path and cannot contribute to the deadlock.
In stock config this removes engineCreated, willStart, didStop and willRelease from the blocking path entirely, including willStart, which appeared in both reported freeze traces. The two hooks LiveKit registers by default (willEnable and didDisable) keep the bounded-wait and request-id safety net from #90.
The flags are written on the JS thread (handler registration) and read on the native audio thread (delegate callbacks), so they are declared atomic. The multi-field request-id state stays under @synchronized because it needs a true critical section.
Scope
Self-contained in this package. The active flags and request ids are internal to react-native-webrtc and never reach app handlers, so the public handler API is unchanged and no changes are needed in @livekit/react-native.
Testing
Refs #89, livekit/client-sdk-react-native#389.