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 ITMGeolocationManager crash #97

Merged

Conversation

ValdasSorys
Copy link
Contributor

Problem:
A crash may occur in ITMGeolocationManager (in our app - for loop in updateWatchers).

Solution:
watchIds isn't read/written to in the same coroutine scope in clearWatch as in other cases in the file. Added mainScope.launch { ... } to clearWatch. After this change I couldn't reproduce the issue.

@ValdasSorys ValdasSorys requested a review from a team as a code owner October 16, 2024 11:24
@CLAassistant
Copy link

CLAassistant commented Oct 16, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@tcobbs-bentley
Copy link
Member

Thank you for submitting this bug fix PR.

If the crash was happening due to updates to watchIds in multiple threads at the same time, then trackPosition should also be updated to make its changes in the main thread.

To be entirely safe, the code that reads from watchIds (onSensorChanged, cancelTasks, resumeLocationUpdates) should also be updated to happen in the main thread. (Alternatively, all access to watchIds could be protected by a ReentrantLock.) I could make all these changes, but not really test them in your app. If you want me to do a first pass update to this PR, let me know. Otherwise, go ahead and make the changes yourself.

@ValdasSorys
Copy link
Contributor Author

The crash is because watchIds is accessed in different threads. I made changes to use ReentrantLock and tested the changes. If you had a different idea to implement this let me know.

@tcobbs-bentley
Copy link
Member

The crash is because watchIds is accessed in different threads. I made changes to use ReentrantLock and tested the changes. If you had a different idea to implement this let me know.

Thanks. Your updated fix looks good, aside from the one comment I left regarding indentation. If there is a reason I'm not understanding for that change, let me know. Otherwise, commit a fix.

This repository is configured to clear approval reviews after every commit, so I'll wait for your response before approving.

@tcobbs-bentley tcobbs-bentley merged commit d253475 into iTwin:main Oct 17, 2024
3 checks passed
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.

3 participants