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

feat(android): optimize TiProperties #13944

Merged
merged 1 commit into from
Jan 11, 2024
Merged

feat(android): optimize TiProperties #13944

merged 1 commit into from
Jan 11, 2024

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Nov 9, 2023

Applying Android Studio hint to use apply instead of commit to fix a potential ANR when saving TiProperties

Consider using apply() instead; commit writes its data to persistent storage immediately, whereas apply will handle it in the background

fixes #13942

SO post: https://stackoverflow.com/a/66717417/5193915
Screenshot of the hint
Screenshot_20231109_170148

@hansemannn
Copy link
Collaborator

If it writes it via a background thread, isn't it possible to run into concurrency / synchronization issues, especially in edge cases?

@m1ga
Copy link
Contributor Author

m1ga commented Nov 13, 2023

According to the SO post commit() still might be worse:

The second problem is with the call to commit(). This performs a synchronous write to the underlying preference file in your SharedPreferences, which blocks the calling thread (in your case, this is again the Main thread). This will also lead to ANRs.

apply():

This ensures the data is stored in memory immediately, while writing the data asynchronously to the underlying preference file. There is still a chance for an ANR however, especially because the queue that Android uses to do the write jobs asynchronously flushes onto the main thread when a receiver completes onReceive() (as well as in other cases). I wouldn't worry about this situation all too much, just something to be aware of.

I don't have any issues with the current commit() part. No ANR in my console, it was an error from Slack, so hopefully we'll get some feedback there if this version is better or not

@hansemannn
Copy link
Collaborator

I would personally not want to risk any breaking changes it the old one worked well. What was the initial reason for the PR?

@m1ga
Copy link
Contributor Author

m1ga commented Nov 13, 2023

I've looked at the file because of the linked issue. Then after Android Studio also suggested to change from commit() to apply() I've made the PR.

SDK tests still run without issues but it will take more testing and feedback to see if this makes any change or introduce other issues

@hansemannn hansemannn merged commit e364209 into master Jan 11, 2024
5 checks passed
@m1ga m1ga deleted the 231109_shared branch January 12, 2024 11:36
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.

ANR on org.appcelerator.titanium.TiProperties.removeProperty (ANR triggered by slow IO operations)
2 participants