Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix crashes from Socket.connect race conditions #260

Closed
wants to merge 4 commits into from

Conversation

KaylaBrady
Copy link

@KaylaBrady KaylaBrady commented Sep 5, 2024

Includes fixes for two flavors of issues we've encountered in our SwiftUI application.

  1. Socket.sendHeartbeat() EXC_BAD_ACCESS (KERN_INVALID_ADDRESS) #253
    This one I'm having trouble reproducing locally, but the Object 0x303285e60 of class HeartbeatTimer deallocated with non-zero retain count 3. This object's deinit, or something called from it, may have created a strong reference to self which outlived deinit, resulting in a dangling reference error especially had me thinking it was an issue with a strong reference in HeartbeatTimer. Please let me know if there is a good way to test this!

  2. "Socket is not connected" error after backgrounding #258 - Error when receiving Error Domain=NSPOSIXErrorDomain Code=57 "Socket is not connected" Error messages

  • Only log failure error messages if self is not nil in PhoenixTransport.receive
  • Disconnect the existing connection in Socket.connect so that old connections are cleaned up.
  • Ignore errors due to normal cancellations

Tested these changes in an app that calls socket.connect() twice whenever returning from the background and confirmed onOpen callback only invoked once and onError not invoked.

@KaylaBrady KaylaBrady marked this pull request as ready for review September 5, 2024 19:01
@ialimz
Copy link

ialimz commented Sep 10, 2024

@ejensen would you please re-review this PR?

Copy link
Collaborator

@dsrees dsrees left a comment

Choose a reason for hiding this comment

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

Thank you for taking this into your own hands and submitting a PR.

  • The weak self ref seems like the appropriate solution
  • I need to test your demo app sample you provided in an Issue before I better understand whats going on with the abnormalError
  • I've left a comment regarding your change to the connect function

@@ -260,6 +260,7 @@ public class Socket: PhoenixTransportDelegate {
paramsClosure: self.paramsClosure,
vsn: vsn)

self.connection?.disconnect(code: CloseCode.normal.rawValue, reason: "connect called")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The connect call is already checking that the socket is not connected before attempting to connect again.

// Do not attempt to reconnect if the socket is currently connected
guard !isConnected else { return }

So in theory, this should never be used. Looking at the JS client (that you linked to in your PR), they only check if the connection exists, not if its been fully connected. So Maybe a better approach here is to update

- guard !isConnected else { return }
+ guard self.connection == nil else { return } 

Copy link
Author

Choose a reason for hiding this comment

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

So in theory, this should never be used.

Where I encountered issues with only guard !isConnected else { return } is calling socket.connect() from both onAppear and onChange(of: scenePhase) when scenePhase is active in a SwiftUI app. On occasion, the socket would be in the connecting state from the first call to connect() and pass the existing guard.

We've since removed the call to connect in onAppear to avoid this race condition entirely.
One concern about guard self.connection == nil else { return } is if the existing connection's state is closing or closed, I would probably expect a new connection to still be opened rather than be left with the existing closed one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I'd advocate for adding an isConnecting var and checking that as well

public var isConnecting: Bool {
    return self.connectionState == .connecting
  }


public func connect() {
    // Do not attempt to reconnect if the socket is currently connected
    guard !isConnecting && !isConnected else { return }
   ...
}

@@ -266,6 +266,11 @@ open class URLSessionTransport: NSObject, PhoenixTransport, URLSessionWebSocketD
// The task has terminated. Inform the delegate that the transport has closed abnormally
// if this was caused by an error.
guard let err = error else { return }

if let urlError = err as? URLError, urlError.code == .cancelled {
Copy link
Collaborator

Choose a reason for hiding this comment

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

From my testing, error is nil when cancelling the task.

@dsrees
Copy link
Collaborator

dsrees commented Oct 10, 2024

5.3.4

@dsrees dsrees closed this Oct 10, 2024
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.

4 participants