Make copying optional when receiving data

[erikprice]

For context, see #424, #426, and #427.

Starting the conversation again on making copying of received frame data optional.

After looking deeper into the calls to handleFrameWithData:opCode: - there are numerous cases, where reading from that data would be incredibly unsafe, since we mutate the data and NSData is not thread-safe.I can see the value from not copying the incoming data, but at the same time using un thread-safe data can lead to crazy crazy problems on the receiving end and at the current moment.

Can you elaborate a bit on where the mutation occurs? I scanned the code, and although the data is passed to numerous delegate methods (any one of which could mutate the data), I don't see where the library itself does so. But I'm probably just not seeing it.

[nlutsenko]

Can you elaborate a bit on where the mutation occurs? I scanned the code, and although the data is passed to numerous delegate methods (any one of which could mutate the data), I don't see where the library itself does so. But I'm probably just not seeing it.

Absolutely, here is the spot where we mutate the currentFrameData.

There is an interesting thing here - it looks like we are safe at not doing any copying for the data, unless it's a control frame, because then we are going to continue adding more data to the current frame data.
I wonder if we remove it completely and only pass in a copy of non-current frame data, or current frame data directly - whether that will work ok or not.

[erikprice]

Is it really as simple as the small change in 6ba8526?

[nlutsenko]

Yes, this is what it looks like. Would love to hear if this solution actually works on a big scale and doesn't cause any problems.

[nlutsenko]

Please rebase and let's test it heavily together.

[erikprice]

Thanks for your help with this. We've updated our app to use this new branch. We'll test in dev and QA for some period of time, and if all goes well there, the changes will end up in our next App Store release. If you leave this PR open, I will follow up here in about a month or so, assuming we don't run into problems before then.

[erikprice]

Also, I assumed this project prefers merge instead of rebase (I merged before I noticed your response), but if you want me to rebase the branch against master instead, just let me know.

[nlutsenko]

merge vs rebase

The way we do it is weird :) As much as possible I rebase the branches, so everything stays in sync and is well-tested on the latest master.
The moment we are landing something onto the master if it contains more than 1 commit and they are concrete - it's merge, otherwise - it's squash/rebase and push.

If you leave this PR open, I will follow up here in about a month or so, assuming we don't run into problems before then.

Sounds perfect! I am also going to try doing some stress testing on this, so we attack this from both ends. If you encounter any issues - please follow-up here. Going to remove review-needed and just keep the PR opened so we can make sure everything is stable before merging.

[erikprice]

Absolutely, here is the spot where we mutate the currentFrameData.

Permalink, for posterity (mainly for my own reference).

[erikprice]

Just wanted to update this issue: it's been a little over a month, and these changes have gone through our QA and been shipped in two App Store releases. So far, we haven't seen any reason to suspect that they cause problems.

[nlutsenko]

Looks like this is good to go!
Amazing performance and memory consumption improvement!
Thank you for bearing with me through the process!