Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correctly cleaning up the websocket stream #5482

Conversation

zhouzh1
Copy link

@zhouzh1 zhouzh1 commented Dec 20, 2024

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.
image

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 zhouzh1 requested review from awsmobilesdk and a team as code owners December 20, 2024 10:09
@zhouzh1
Copy link
Author

zhouzh1 commented Dec 20, 2024

@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
Copy link
Author

zhouzh1 commented Dec 24, 2024

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

@ruisebas
Copy link
Member

ruisebas commented Dec 24, 2024

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!

@ruisebas ruisebas closed this Dec 24, 2024
@zhouzh1
Copy link
Author

zhouzh1 commented Dec 25, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants