Fix ARTChannel.setOptions and ARTRealtimeChannel.setOptions: Recreate ARTDataEncoder on ARTChannel options update

[lukasz-szyszkowski]

No description provided.

[ben-xD]

Thanks for quickly working on this :)

Unfortunately, I find that the application crashes now when I call [channel setOptions:channelOptions];. I've recorded this in a video that I attached. Notice that the application is paused at a breakpoint before calling [channel setOptions:channelOptions];. After playing, it immediately crashes at a line of code related to this PR.

CleanShot.2021-11-04.at.23.49.11.2.mp4

[lukasz-szyszkowski]

Thanks for quickly working on this :)

Unfortunately, I find that the application crashes now when I call [channel setOptions:channelOptions];. I've recorded this in a video that I attached. Notice that the application is paused at a breakpoint before calling [channel setOptions:channelOptions];. After playing, it immediately crashes at a line of code related to this PR.

CleanShot.2021-11-04.at.23.49.11.2.mp4

fixed here 0a7fd0c

[ben-xD]

Thanks. I tested these changes and there don't seem to be any crashes anymore. The crash turned out to be a race condition issue, as when I was testing it, it worked fine if using the debugger and stepping through the program more slowly. This led to @lukasz-szyszkowski trying multiple fixes, and we finally settled on dispatch_barrier_async as it stopped causing the crashes.

I would appreciate others with more objective-C and especially those with objective-C + concurrency eyes here to approve if the race condition is fully fixed, not just a fix for some/most cases.

[lawrence-forooghian]

I think I'd like to better understand the motivation behind the synchronisation that you've added here. Could you please explain a little what you think was the cause of the crash that @ben-xD was seeing, and how this fixes it?

Perhaps I could also check that I've understood the meaning of the _nosync method pattern that I've seen through the codebase. Is the idea of these methods that they should only be called on the SDK's internal queue, and hence shouldn't need any sort of synchronisation? If so, then shouldn't that mean that -setOptions_nosync: here is already being called on _queue and hence doesn't need to be dispatched there?

[maratal]

@ben-xD can you reproduce this crash one more time and type "po [address]" in the debug console, just curious what object hides under that address 0x*****8080. Triggering any synchronisation in all *_nosync: methods doesn't look good. Probably previous commit is more suitable for this fix, but we need to understand the nature of this crash better.

[ben-xD]

@maratal, after trying to reproduce the issue, the crashes no longer happened. I also reported this in slack: https://ably-real-time.slack.com/archives/CSQEKCE81/p1636137481110900?thread_ts=1636104128.094400&cid=CSQEKCE81

Therefore, the latest state of the PR looks good to merge, without any of the thread safety additions.

[lukasz-szyszkowski]