Skip to content

Update NWWebSocket.swift#43

Closed
ajinumoto wants to merge 1 commit intopusher:mainfrom
ajinumoto:main
Closed

Update NWWebSocket.swift#43
ajinumoto wants to merge 1 commit intopusher:mainfrom
ajinumoto:main

Conversation

@ajinumoto
Copy link

Fix multiple NWConnection during migrateConnection

Fix multiple NWConnection during migrateConnection
@ajinumoto
Copy link
Author

Problem:
Every time migrateConnection() is called, new NWConnection will be created. The problem appear if migrateConnection() is called multiple time, the previous NWConnection from migratedConnection variable is never closed and multiple NWConnection will be alive.

Solution
change migratedConnection to global variable and make sure its cancel() and nil first before creating new one.

private func migrateConnection(completionHandler: @escaping (Result<WebSocketConnection, NWError>) -> Void) {

let migratedConnection = NWConnection(to: endpoint, using: parameters)
migratedConnection?.cancel()

Choose a reason for hiding this comment

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

if migrateConnection was already called and successfully ended, in the next call when you cancelling migratedConnection you cancelling current 'connection'

@Keith-wright
Copy link
Contributor

Thanks for raising this @ajinumoto. The issue of multiple NWConnection instances being created during migration was addressed in #56, which introduced a guard at the top of migrateConnection that prevents re-entrant calls:

guard !isMigratingConnection else {
    completionHandler(.failure(NWError.posix(.EALREADY)))
    return
}

The same PR also introduced explicit cleanup of the old connection's handlers before cancelling it with a short delay, ensuring no stale callbacks fire during the transition. Closing this in favour of that work.

If you're still able to reproduce the issue on the latest release (0.5.10), please open a new issue and we'll take another look.

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.

3 participants