Skip to content

Fix data races on lazily initialized members outside Room - #1089

Open
tarsyang wants to merge 1 commit into
livekit:mainfrom
tarsyang:fix/lazy-init-races-outside-room
Open

Fix data races on lazily initialized members outside Room#1089
tarsyang wants to merge 1 commit into
livekit:mainfrom
tarsyang:fix/lazy-init-races-outside-room

Conversation

@tarsyang

Copy link
Copy Markdown

Follow-up to #1088, which covered Room. This sweeps the remaining lazy vars in Sources/LiveKit and fixes the four owners whose first access can happen concurrently from contexts that don't serialize with each other. Same failure mode as before: two threads see empty storage, both run the initializer, and one of the two objects is dropped. Because the WebRTC delegate properties involved are all weak, the dropped object doesn't just leak, whatever was registered through it goes silent.

Fixed

  • DeviceManager.discoverySession: on iOS/tvOS init dispatches two blocks to the global utility queue and each one touches the session first. TSan on the iOS simulator reports a write/write race between the two blocks, i.e. two DiscoverySessions get created and the two KVO observations can land on different ones. Now a single block creates the session once, retains it, and registers both observations. Creation deliberately stays off the caller's thread: discovery session setup blocked for over 10 s in the xctest host on my machine (devices() timed out), so making this an eager let in init was not an option.
  • AudioManager.capturePostProcessingDelegateAdapter / renderPreProcessingDelegateAdapter: reachable from the public delegate setters and from add/remove(localAudioRenderer:) / add/remove(remoteAudioRenderer:) (also via LocalAudioTrack.add(audioRenderer:)) on any thread. Their initializers don't capture self, so they are plain lets now, like metricsManager in Fix data races on Room's lazy member initialization #1088. If the losing adapter had already attached itself as the (weak) APM delegate, the processing delegate or renderer registered through it stops receiving buffers once the stored one attaches.
  • CameraCapturer.adapter / capturer: on iOS 16+/tvOS 17+ the isMultitaskingAccessSupported log line in init forces both, but on macOS, Mac Catalyst, visionOS and older iOS the first touch is startCapture(), stopCapture() or the public captureSession getter, from whatever thread calls them. With the wrong interleaving the retained LKRTCCameraVideoCapturer ends up with a (weak) delegate nobody retains, and frames never reach the track. Both are created in init now, with the adapter's back-reference set after super.init. This is the one observable behavior change: on the platforms listed above the AVCaptureSession now exists from track creation instead of first use, which is what RTCCameraVideoCapturer itself intends (its own init comment: the app may want the session for a preview layer before starting).
  • E2EEManager.delegateAdapter: first touched in addRtpSender/addRtpReceiver, reachable from setup(room:) on the caller's thread and from RoomDelegate callbacks on the room's delegate queue. The in-SDK setup(room:) in Room.connect() runs right after cleanUp() and finds no publications, but setup(room:) and Room.e2eeManager are public and can run against a live room, where the two paths race. A frame cryptor whose (weak) delegate was the losing adapter never reports state changes. Now a let with the target set after super.init. Not reproduced under TSan (needs a live room with a frame cryptor); fixed by inspection.

Left as they are

  • SignalClient._requestQueue / _responseQueue, Transport._iceCandidatesQueue, TranscriptionStreamReceiver.partialMessages: actor-isolated.
  • VideoView._pinchGestureRecognizer: main actor.
  • OSLogger.rtcLogger: touched only in init and deinit.
  • LKSampleHandler.log: forced in init.
  • RemoteAudioTrack._adapter: forced in init (audioTrack.add(_adapter)); could be a let for consistency, but there is no race to fix, so left untouched.

Verification

  • A temporary probe suite (fresh instance, DispatchQueue.concurrentPerform on the first access; not part of this PR) under -enableThreadSanitizer YES. On main it reports data races in AudioManager.capturePostProcessingDelegateAdapter.getter, AudioManager.renderPreProcessingDelegateAdapter.getter, CameraCapturer.adapter.getter (macOS) and DeviceManager.discoverySession.getter (iOS simulator, write/write between the two init closures). On this branch the same probe reports nothing on macOS and iOS, and DeviceManager.devices() / multiCamCompatibleDevices(for:) resolve on the simulator.
  • swiftformat --lint, swiftlint --strict, swift build, and RoomTests / PublishTrackTests / PublishBufferCapturerTests / EncryptedDataChannelTests against a local livekit-server --dev (all rooms in TestEnvironment run with encryption options, so E2EEManager and the frame cryptor path are exercised).

Follow-up to livekit#1088. The remaining lazy members whose first access is
not serialized are now created exactly once:

- DeviceManager: one utility-queue block creates the discovery session,
  retains it and registers both KVO observations. Two blocks each
  touching the lazy session raced on iOS/tvOS.
- AudioManager: the two processing delegate adapters are plain lets;
  their initializers capture nothing.
- CameraCapturer: adapter and RTC capturer are created in init, with
  the adapter's back-reference set after super.init.
- E2EEManager: the frame cryptor delegate adapter is a let with its
  target set after super.init.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@pblazej pblazej mentioned this pull request Aug 25, 2026
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