search

tidev / titanium-sdk

🚀 Native iOS and Android Apps with JavaScript
https://titaniumsdk.com/
Other
2.76k stars 1.21k forks source link

feat(android): optimize TiProperties #13944

Closed m1ga closed 10 months ago

m1ga commented 1 year ago

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 https://github.com/tidev/titanium-sdk/issues/13942

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

hansemannn commented 1 year ago

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

m1ga commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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