⚡ Offload network operations and parsing to background queue in IPtalkManager#6
⚡ Offload network operations and parsing to background queue in IPtalkManager#6SilentMalachite wants to merge 1 commit into
Conversation
…o background queue - Introduce `networkQueue` (userInitiated) for NWListener and NWConnection operations. - Make parsing methods `nonisolated` to run on background threads. - Update `receiveData` to parse data on the background queue before dispatching to MainActor for UI updates. - Ensure thread safety for all state properties. Co-authored-by: SilentMalachite <84108752+SilentMalachite@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR moves IPtalkManager’s Network.framework callbacks and packet parsing off the main thread by introducing a dedicated DispatchQueue, keeping @MainActor work focused on UI-facing state updates.
Changes:
- Added a dedicated
networkQueueand startedNWListener/NWConnectionon it instead of.main. - Refactored receive handling to parse packets off-main and then hop to
@MainActorto applyreceivedTextupdates. - Marked packet create/parse helpers as
nonisolatedand switched encoding storage to a static constant.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let data = content, let self = self { | ||
| // Parse off the main thread | ||
| if let packet = self.parseIPtalkPacket(data: data) { | ||
| Task { @MainActor in | ||
| self.processReceivedPacket(packet) | ||
| } |
There was a problem hiding this comment.
receiveData(from:) now schedules a new Task { @MainActor … } per packet, but the receive loop continues immediately on the network queue (the next receiveMessage is set up right after the handler returns). This removes the implicit backpressure that previously existed when the loop continued from the MainActor task, and can lead to a large backlog of pending MainActor tasks under high traffic. Consider adding explicit backpressure/batching (e.g., coalescing text updates) or resuming the receive loop only after the MainActor update completes if ordering/throttling is required.
| } | ||
|
|
||
| listener?.start(queue: .main) | ||
| listener?.start(queue: networkQueue) |
There was a problem hiding this comment.
The networking/concurrency change (starting the listener/connection on networkQueue instead of .main) isn’t covered by tests in SummaryTalkTests (current tests only validate packet create/parse). Adding an XCTest that asserts network callbacks/parsing aren’t executed on the main thread (and that UI state updates still occur on the MainActor) would help prevent regressions.
💡 What:
Moved network operations (listening, connecting, receiving, broadcasting) and data parsing logic in
IPtalkManagerfrom the Main Thread to a dedicated backgroundDispatchQueue.🎯 Why:
The previous implementation forced all network I/O and data parsing (Shift-JIS decoding and command processing) onto the Main Thread using
.start(queue: .main). This could potentially block the UI during heavy network traffic or complex parsing. Offloading these tasks ensures the UI remains responsive.📊 Measured Improvement:
Verified via code analysis and strict concurrency checks. Network callbacks and parsing now execute on a background queue. The Main Actor is only accessed for updating UI state (
receivedText,isConnected, etc.), minimizing the workload on the main thread.PR created automatically by Jules for task 6958964418284458593 started by @SilentMalachite