PhoenixTransport Crash Fix for iOS 18 #275
Conversation
…ead of completionHandler
varun-score
changed the title
|
|
||
|
|
||
| /// Holds the current receive task | ||
| private var receiveMesageTask: Task<Void, Never>? { |
There was a problem hiding this comment.
Fixed, thank you for reviewing the PR!
|
This is a race, right? Crashes if a message is received during teardown? (Note that the original reporter in the devforums thread from #267 just wrote back in and that was the culprit - they closed their CFStream on one thread while using it on another). Anyway, this seems like a reasonable way to fix this issue. But it's definitely not the only thread safety issue in the library, and would be nice to clean concurrency up in a more structured way. Maybe |
|
One issue with using |
|
This should be fine since URLSessionTransport is only available to iOS 13 and above. However, we might want to explore a more structured approach. While using an actor could be a good option, it cannot directly conform to URLSessionWebSocketDelegate because the protocol is obj-c based and actor types are not obj-c compatible. |
|
Oh, nice! I missed that your changes are already contained within an @available. And great point on the ObjC protocol. |
| @@ -192,6 +198,7 @@ open class URLSessionTransport: NSObject, PhoenixTransport, URLSessionWebSocketD | |||
|
|
|||
| deinit { | |||
| self.delegate = nil | |||
| receiveMessageTask?.cancel() | |||
There was a problem hiding this comment.
Should the receiveMessageTask also be canceled on disconnect?
There was a problem hiding this comment.
Thank you for reviewing the PR! I believe self.task?.cancel in disconnect should handle canceling the receiveMessageTask, but to be on the safer side, I’ve explicitly canceled it in disconnect as well.
| @@ -231,6 +238,7 @@ open class URLSessionTransport: NSObject, PhoenixTransport, URLSessionWebSocketD | |||
| } | |||
|
|
|||
| self.readyState = .closing | |||
| receiveMessageTask?.cancel() | |||
There was a problem hiding this comment.
Just to be safe, lets place this after self.session?.finishTaskAndInvalidate. I would hate to kill the Task and then potentially miss something that comes in before the connection has actually closed. I haven't tested to ensure that this wouldn't happen but I'm just being precautionary
There was a problem hiding this comment.
Sounds good!. Moved cancellation after finishTaskAndInvalidate.
|
Hey @dsrees, thank you for approving the PR. Could you please create a release? |
Problem
Since the release of iOS 18, we've been experiencing a significant number of crashes related to WebSocket connections. The issue was first reported in October 2024 (Issue #267) and appears to be specifically related to the
NSURLSessionWebSocketTask.receive(completionHandler:)method.Solution
This approach aligns with the solution implemented by SendBird for a similar issue in their SDK (reference).
Testing and Validation
Looking forward to your review and feedback. This should resolve the iOS 18 crash issues. 🚀