Skip to content

Cancel removed Network connections - #40

Closed
rchatham wants to merge 2 commits into
feature/network-migration-docs-guidefrom
feature/network-migration-connection-cleanup
Closed

Cancel removed Network connections#40
rchatham wants to merge 2 commits into
feature/network-migration-docs-guidefrom
feature/network-migration-connection-cleanup

Conversation

@rchatham

@rchatham rchatham commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Summary

Makes Network connection teardown explicit when receive loops end or coordinator state removes a connection.

Changes

  • Cancel NWConnection when the receive loop sees an error or EOF before emitting the synthesized .cancelled state.
  • Cancel pending or registered coordinator connections when removeConnection(_:) removes them from coordinator state.
  • Keep cancellation outside the coordinator serial queue so cancellation-driven callbacks can safely re-enter cleanup without deadlocking.
  • Add coordinator coverage proving pending removals cancel without emitting a session event.
  • Extend registered removal coverage to verify the removed connection is cancelled.

Verification

  • swift test --filter NetworkPeerCoordinatorTests — 18 tests passed
  • swift test — 109 tests passed
  • xcodebuild test -project PeerConnectivity.xcodeproj -scheme PeerConnectivity -destination 'platform=iOS Simulator,name=iPhone 16,OS=18.3.1' -configuration Debug — 109 tests passed
  • GitHub Actions pull_request CI — Swift Package Tests and Xcode Project Tests passed
  • GitHub Actions push CI — Swift Package Tests and Xcode Project Tests passed
  • Correctness reviewer — no merge-blocking findings
  • Security reviewer — no merge-blocking security findings

Notes / limitations

  • This keeps cleanup idempotent; duplicate .cancelled callbacks from Network.framework remain safe no-ops after the first removal.
  • No public API changes.

Stack context

Stacked on PR #39 (feature/network-migration-docs-guide). This is the explicit Network connection cleanup slice of the migration stack.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens Network.framework connection teardown semantics so NWConnection instances are explicitly cancelled when a receive loop terminates (EOF/error) and when the coordinator removes a pending or registered connection, while avoiding coordinator-queue deadlocks from cancellation-driven callbacks.

Changes:

  • Cancel NWConnection when the receive loop hits EOF/error before synthesizing a .cancelled state.
  • Update NetworkPeerCoordinator.removeConnection(_:) to cancel pending/registered connections after removing them from coordinator state (and outside the coordinator serial queue).
  • Extend coordinator unit tests to assert removed connections are cancelled and that pending removals do not emit session events.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
Sources/NetworkPeerTransport.swift Cancels the underlying NWConnection when the receive loop ends due to EOF/error before emitting the synthesized .cancelled state.
Sources/NetworkPeerCoordinator.swift Ensures removing a connection also cancels it, with cancellation occurring outside the coordinator queue to avoid deadlock risk.
PeerConnectivityTests/NetworkPeerCoordinatorTests.swift Adds/extends coverage verifying cancellations for both registered and pending connection removals (and no session event for pending removal).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@rchatham

Copy link
Copy Markdown
Owner Author

Superseded by consolidated PR #42. No commits were discarded; this branch is preserved as a recovery and reference point. Addressed review findings were replied to and resolved before consolidation.

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