fix: correctly cleaning up the websocket stream

[zhouzh1]

Issue #, if available:
None

Description of changes:
The issue we're suffering from:
Currently we found there're many crashes in our iOS app which are related to the AWSIoT lib when it's cleaning up the websocket streams, after dig into the relevant source code, I think the possible reason for those crashes is that the lib code doesn't put enough consideration for the situation where the created websocket stream is shared by mutiple threads, meanwhile, more than one thread have the method to perform the cleaning job for that websocket stream, and moreover, they don't check the stream status, that would cause the same websocket stream to be cleaned more than one time, then the app crash happens. The below screenshot shows some crash reports.

The changes implemented in this PR

• Imposing stricter status check before cleaning the associated stream object, to ensure the stream will only be cleaned one time.

Check points:

• Added new tests to cover change, if needed
• All unit tests pass
• All integration tests pass
• Updated CHANGELOG.md
• Documentation update for the change if required
• PR title conforms to conventional commit style

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[zhouzh1]

@awsmobilesdk Hi guys, anyone can help take a look at this PR? Currently the corresponding issues are causing many app crashes, but I am not sure if my change makes sense, please help confirm, thanks.

[zhouzh1]

@ruisebas @thisisabhash Could you help take a look at this PR?

[sebaland]

Hi @zhouzh1, thanks for opening this PR!

Unfortunately, the validations you've added will not prevent concurrency issues/crashes, since they can still be accessed by multiple threads at the same time.

However, we've recently added several concurrency checks in version 2.38.1 that might prevent your crashes. Would you mind checking that version up?

If after upgrading you're still getting crashes, please submit an issue and provide the full symbolicated crashlog and we'll take a look.

Thanks!

[zhouzh1]

@ruisebas Thanks for the reply, I will retry the 2.38.1. As for the changes in my PR, yes I didn't prevent the relevant objects from being accessed by mutiple threads, I'm just trying to prevent their relavant methods from being invoked mutiple times by mutiple threads. Anyway, do you think if my changes would cause any possible new issues? I'm thinking that if it won't cause new issues, maybe I can try it by patching the lib if the new 2.38.1 vesion still couldn't help. Thanks.

[sebaland]

The problem is that even with your changes, the streams close method could still be invoked multiple times by multiple threads.

For example, you've added these checks for the value of streamStatus:

(stream.streamStatus != NSStreamStatusNotOpen && stream.streamStatus != NSStreamStatusClosed)

Let's say the status is NSStreamStatusOpen and Thread A tries to close the stream. The check resolves to true, as the status is neither NSStreamStatusNotOpen nor NSStreamStatusClosed, so it proceeds to invoke [stream close].

But right at that same time, another Thread B attempts to close the stream. The check will still resolve to true because Thread A hasn't finished its execution of the closure method and the stream's status hasn't changed, so now Thread B will also invoke [stream close].

[zhouzh1]

@ruisebas What you said above definitely make sense, so the checks I added include the check for the delegate flag property, if my hypothesis for this crash issue is right, that should still be able to make it? Also, do you think my changes would cause new issues? Thx.

[sebaland]

It's the same situation, two threads could read the delegate property around the same time, which would still be non-nil and then both threads would proceed with the execution.

Having said that, I don't think your changes would cause new issues, actually they could help reduce the concurrency troubles. But they don't really properly fix the underlying problem.

Are you still getting crashes/issues with the changes implemented in version 2.38.1?

[zhouzh1]

@ruisebas Thanks for the reply, and I agree with what you said that my changes aren't able to 100% resolve the issue, I haven't really tried the 2.38.1 yet, because my new version of the app hasn't been released yet, as for why I keep asking that, it's because of I reviewed the relevant code, seems that the 2.38.1 still couldn't totally eliminate those concurrency issues, but anyway, that's just my suspicion, so let me try it at first, if it doesn't help, I will try to improve my changes and ship them by patching way.