Fix rare crashes in IOSBackgroundTaskScheduler with on entering callbacks #3751
Conversation
laevandus
added
🐞 Bug
|
""" WalkthroughThread-safety was introduced to the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Thread(s)
participant Scheduler as IOSBackgroundTaskScheduler
participant Queue as Serial Queue
loop 100 iterations (concurrent)
alt Even index
Test->>Queue: async { assign onEnteringBackground/onEnteringForeground }
Queue->>Scheduler: set closure
else Odd index
Test->>Queue: async { clear onEnteringBackground/onEnteringForeground }
Queue->>Scheduler: clear closure
end
end
Note over Scheduler: All closure assignments/clears are serialized via Queue
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/StreamChatTests/WebSocketClient/BackgroundTaskScheduler_Tests.swift (1)
78-87: Effective crash test for race condition validation.This concurrency test effectively reproduces the conditions that would trigger the original race condition. The alternating pattern of starting and stopping app state updates with concurrent execution is well-designed to stress test the thread safety improvements.
The absence of assertions is appropriate for a crash test - the success condition is simply not crashing.
Consider adding a brief comment explaining this is a crash test:
+ // Crash test: Verifies no race conditions when concurrently starting/stopping app state updates func test_callingAppStateUpdatesConcurretly() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/StreamChat/WebSocketClient/BackgroundTaskScheduler.swift(3 hunks)Tests/StreamChatTests/WebSocketClient/BackgroundTaskScheduler_Tests.swift(1 hunks)
🔇 Additional comments (4)
Sources/StreamChat/WebSocketClient/BackgroundTaskScheduler.swift (4)
85-88: LGTM: Proper serialization of callback assignment.The use of
queue.asyncfor callback assignment prevents race conditions during concurrent calls tostartListeningForAppStateUpdates. The asynchronous dispatch ensures the assignment doesn't block the caller while maintaining thread safety.
106-109: LGTM: Consistent callback clearing pattern.The callback clearing follows the same async pattern as assignment, ensuring thread-safe cleanup of callbacks when stopping app state updates.
125-126: LGTM: Safe callback invocation pattern.The synchronous read of the callback followed by invocation outside the queue is the correct pattern. This prevents blocking the serial queue during callback execution while ensuring thread-safe access to the callback closure.
130-131: LGTM: Consistent foreground callback pattern.This follows the same safe invocation pattern as the background callback handler, maintaining consistency and thread safety.
| @objc private func handleAppDidEnterBackground() { | ||
| onEnteringBackground() | ||
| let callback = queue.sync { onEnteringBackground } | ||
| callback() |
There was a problem hiding this comment.
Running the callback on the calling thread (not on the queue)
SDK Size
|
|
@laevandus can you also fix this in StreamCore? |
Public Interface🚀 No changes affecting the public interface. |
Public Interface🚀 No changes affecting the public interface. |
1 similar comment
Public Interface🚀 No changes affecting the public interface. |
|
🔗 Issue Links
Resolves: https://linear.app/stream/issue/IOS-983
🎯 Goal
Rare crashes with mutating onEntering callback mutations
📝 Summary
🛠 Implementation
🎨 Showcase
🧪 Manual Testing Notes
N/A
☑️ Contributor Checklist
docs-contentrepoSummary by CodeRabbit