Add thread safety to local push reconnection logic #4132
Conversation
- Introduce serial DispatchQueue for synchronizing access to disconnectedServers - Wrap all disconnectedServers access in queue.sync blocks - Ensure timer methods (scheduleReconnection, attemptReconnection, cancelReconnection) run on main thread with dispatchPrecondition - Use DispatchQueue.main.async to hop to main thread before calling timer methods from status() - Fix deinit to safely invalidate timer on main thread - Add comprehensive comments documenting thread safety requirements Co-authored-by: bgoncal <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds thread safety mechanisms to the local push notification reconnection logic to prevent race conditions when status() is called from multiple threads via sync state observers and KVO callbacks.
Key Changes:
- Introduced a serial
DispatchQueueto synchronize access to thedisconnectedServersshared mutable state - Added
dispatchPreconditionchecks to enforce main thread execution for timer-related operations - Modified
deinitto safely invalidate the reconnection timer on the main thread
| DispatchQueue.main.async { [weak self] in | ||
| self?.cancelReconnection() | ||
| } |
There was a problem hiding this comment.
Using DispatchQueue.main.async inside queue.sync can lead to deadlocks if the serial queue is blocked waiting for the main queue, while the main queue needs to acquire the serial queue lock. Consider restructuring this code to release the queue lock before dispatching to main, or move the decision logic outside the sync block.
| private let queue = DispatchQueue(label: "io.homeassistant.LocalPushInterface") | ||
|
|
||
| // Reconnection timer properties | ||
| // These properties must only be accessed on the main queue since Timer.scheduledTimer requires main thread |
There was a problem hiding this comment.
Comment states "These properties must only be accessed on the main queue since Timer.scheduledTimer requires main thread", but this is inaccurate. Timer.scheduledTimer does not require the main thread - it requires the thread with an active RunLoop. Timers can be scheduled on any thread with a RunLoop, though the main thread is most common for UI-related code.
| // These properties must only be accessed on the main queue since Timer.scheduledTimer requires main thread | |
| // These properties must only be accessed on a thread with an active RunLoop (typically the main queue), | |
| // since Timer.scheduledTimer relies on a RunLoop rather than strictly requiring the main thread |
| let serverInfo = queue.sync { () -> (count: Int, identifiers: [String]) in | ||
| (disconnectedServers.count, disconnectedServers.map(\.rawValue)) | ||
| } |
There was a problem hiding this comment.
The pattern of creating a tuple with (count: Int, identifiers: [String]) from disconnectedServers is repeated in multiple locations (lines 186-188, 216-218, 265-267, 309-311). Consider extracting this into a private helper method to reduce code duplication and improve maintainability.
| } | ||
|
|
||
| Current.Log.verbose("Current disconnected servers: \(disconnectedServerIds)") | ||
| Current.Log.verbose("Reconnection attempt count: \(reconnectionAttempt)") |
There was a problem hiding this comment.
Accessing reconnectionAttempt in updateManagers() without thread safety. This method is called from background thread (NEAppPushManager.loadAllFromPreferences callback at line 272), but reconnectionAttempt is a main-thread-only property. This creates a race condition when the timer fires on the main thread and modifies reconnectionAttempt at the same time.
| DispatchQueue.main.async { [reconnectionTimer] in | ||
| reconnectionTimer?.invalidate() |
There was a problem hiding this comment.
The deinit captures reconnectionTimer in a closure that is dispatched asynchronously to the main queue. If deinit is called from a background thread and the object is deallocated before the async block executes, the captured timer reference may become invalid. Consider using a pattern that safely invalidates the timer synchronously on the main thread if already on main, or dispatch sync to main if on a background thread.
| DispatchQueue.main.async { [reconnectionTimer] in | |
| reconnectionTimer?.invalidate() | |
| if Thread.isMainThread { | |
| reconnectionTimer?.invalidate() | |
| } else { | |
| let timer = reconnectionTimer | |
| DispatchQueue.main.sync { | |
| timer?.invalidate() | |
| } |
| DispatchQueue.main.async { [weak self] in | ||
| self?.scheduleReconnection() | ||
| } |
There was a problem hiding this comment.
Using DispatchQueue.main.async inside queue.sync can lead to deadlocks if the serial queue is blocked waiting for the main queue, while the main queue needs to acquire the serial queue lock. Consider restructuring this code to release the queue lock before dispatching to main, or move the decision logic outside the sync block.
|
Found 1 unused localization strings in the codebase. Click to see detailsTo clean up these strings, manually remove them from the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## local-push #4132 +/- ##
=============================================
Coverage ? 45.52%
=============================================
Files ? 242
Lines ? 13736
Branches ? 0
=============================================
Hits ? 6253
Misses ? 7483
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
The
status()method inNotificationManagerLocalPushInterfaceExtensioncan be called from multiple threads via sync state observers and KVO callbacks, but was accessing shared mutable state without synchronization. This creates race conditions when multiple threads simultaneously modifydisconnectedServersor schedule/cancel timers.Changes:
DispatchQueueto synchronize all access todisconnectedServersdispatchPreconditionDispatchQueue.main.asyncwhen hopping from background to main threadKey implementation details:
@MainActorannotation sincestatus()is a synchronous protocol methodreconnectionTimer,reconnectionAttempt) remain main-thread-onlydisconnectedServersreads/writes wrapped inqueue.syncblocksdeinitto safely invalidate timer on main threadScreenshots
N/A - internal logic change with no UI impact
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes
This PR merges into #4113 to address feedback on thread safety from the code review.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.