pusher / NWWebSocket

A WebSocket client written in Swift, using the Network framework from Apple.
MIT License
123 stars 25 forks source link

Fix MemoryLeak in NWWebSocket #39

Closed choulepoka closed 1 year ago

choulepoka commented 1 year ago

Problem: The dispatch work item created create a strong reference to self, which in turn have a strong reference on disconnectionWorkItem, creating an infinite retain cycle that makes the NWWebSocket an immortal object

This is a major memory leak for apps that try arbitrarily disconnect from the websocket (i.e.: a logout) and then reconnect later (i.e.: a login)

This cause multiple socket to be alive, and taking up a sizeable amount of system resources

Solution Add the [weak self] in clause in the dispatch work item declaration, and add a guard let self else { return } for the case where self is truly deallocated.

The solution has been tested, and resolved the memory leak with no adverse side-effect

See https://github.com/pusher/NWWebSocket/pull/38

danielrbrowne commented 1 year ago

@benjamin-tang-pusher I'm the original author of this SDK. I just spotted this issue report - I think this will need some additional testing with the integration into the Channels Swift SDK but I think this is quite an important fix to merge in and get released as v0.5.3 and then also included in a patch release of the Channels Swift SDK ASAP.

For what it's worth this is also likely to be the root cause for #33

choulepoka commented 1 year ago

For what it's worth, we've been running a fork with the aforementioned bug fix for the last 3 weeks with around 10k users, and we had no negative side effects in term of functionality.

And our memory related crash rate improved significantly over the same period.

benjamin-tang-pusher commented 1 year ago

Thanks @choulepoka for the PR, I will give it a test.

benjamin-tang-pusher commented 1 year ago

https://github.com/pusher/NWWebSocket/pull/38 has been released, and will be picked up by our https://github.com/pusher/pusher-websocket-swift library as well.

Thanks @choulepoka for the contribution.