Crashes in NWWebsocket

[theolampert]

We're seeing intermittent crashes in production in both our iOS and macOS clients that use this library. The majority of them look something like the following:

EXC_BAD_ACCESS: Attempted to dereference garbage pointer 0xc4f6a150.

0  libobjc.A.dylib +0x25b4         _objc_retain_x23
1  Network +0x603bc                _nw_connection_report_state_with_handler_on_nw_queue
2  Network +0x5f100                ___nw_connection_start_block_invoke
3  libdispatch.dylib +0x2138       __dispatch_call_block_and_release
4  libdispatch.dylib +0x3dd0       __dispatch_client_callout
5  libdispatch.dylib +0xb3fc       __dispatch_lane_serial_drain
6  libdispatch.dylib +0xbf60       __dispatch_lane_invoke
7  libdispatch.dylib +0xd280       __dispatch_workloop_invoke
8  libdispatch.dylib +0x16cb0      __dispatch_root_queue_drain_deferred_wlh
9  libdispatch.dylib +0x16524      __dispatch_workloop_worker_thread
10 libsystem_pthread.dylib +0x4930 __pthread_wqthread

It looks as if they happen after a certain period of time. We see them much more frequently on macOS where the client is left open for hours or days. They appear to be originating from the underlying NWWebsocket library.

Is there a reason to manage the websocket connection manually anymore with the introduction of URLSessionWebSocketTask?

I have been experimenting with a fork of the library that switches this out and was able to get the pusher-websocket-swift library passing all its tests with a few changes:

pusher/NWWebSocket@main...theolampert:NWWebSocket:main
https://github.com/theolampert/pusher-websocket-swift/tree/nwwebsocket-fork

Would moving to URLSessionWebSocketTask be of interest to this library? If so I could prepare a draft PR here. The client becomes small enough that it could even be inlined into this package, making contributions easier and I feel like it might avoid a whole host of issues with managing the connection manually.

[aonemd]

Hey @theolampert , I'm unfamiliar with the problem but I'd be happy to review and test your changes. Please go ahead open the PR (it might take some time to get released due to the holidays but I'll also send it to the rest of the team to review). Thank you 🙏

[theolampert]

@aonemd No worries, I'll draft something up in the next week, and we'll also test this ourselves internally.

[theolampert]

We're still testing internally, we found another area in our code that might be responsible and are testing this. I'll open a draft PR if we're able to narrow this down.

[theolampert]

So, we eventually deployed my fork on both apps and aren't seeing the crash anymore, they've been running for about a week, I've also been leaving one client open for long periods and periodically testing and don't seem to have any of the reconnection issues mentioned in other issues.

See #442 and let me know your thoughts.