Fix #5093 : Crash on api below 24 #5211
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,13 +55,28 @@ class AndroidLocaleFactory @Inject constructor( | |
| */ | ||
| fun createAndroidLocale(localeContext: OppiaLocaleContext): Locale { | ||
| // Note: computeIfAbsent is used here instead of getOrPut to ensure atomicity across multiple | ||
|
There was a problem hiding this comment. Given the problems that this has caused in earlier Android API versions, perhaps we should outright prohibit it to ensure this can't happen again. I suggest adding a static regex pattern + test to check for this. See: https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks#generic-regex-pattern-matching-against-file-contents. There was a problem hiding this comment. Checked the suggested approach : (https://gist.github.com/BenHenning/deb77757e89e1974066a6e76d1496f8b/revisions) I have conducted a test on kotlin playground with suggested approach using this docs : (https://kotlinlang.org/docs/shared-mutable-state-and-concurrency.html) But synchronization doesn't seem to be achieved . You could see the test here : (https://pl.kotl.in/cgtBwrYAb)
What do you suggest ? There was a problem hiding this comment. Thanks @Shaik-Sirajuddin. I think we would need to run the checks in a carefully isolated environment (vs. a cloud environment) in order to be confident in the results, but it does make sense why a fine-grained confinement strategy like blocking dispatcher would have poorer performance over a mutex (especially a cooperative one like Kotlin's I generally lean toward maintaining the existing pattern for properly solving this (which is blocking dispatcher as we use that everywhere else). I'm actually not opposed to using I'm open to either |
||
| // threads calling into this create function. ( computeIfAbsent is introduced in API Level 24 ) | ||
| return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | ||
| memoizedLocales.computeIfAbsent(localeContext) { | ||
| val chooser = profileChooserSelector.findBestChooser(localeContext) | ||
| val primaryLocaleSource = LocaleSource.createFromPrimary(localeContext) | ||
| val fallbackLocaleSource = LocaleSource.createFromFallback(localeContext) | ||
| val proposal = chooser.findBestProposal(primaryLocaleSource, fallbackLocaleSource) | ||
| return@computeIfAbsent proposal.computedLocale | ||
| } | ||
| } else { | ||
| // Note : Using get/PutIfAbsent For API Level below 24 as computeIfAbsent is introduced in API Level 24 | ||
| val locale = memoizedLocales[localeContext] | ||
| return if (locale != null) { | ||
| locale | ||
| } else { | ||
| val chooser = profileChooserSelector.findBestChooser(localeContext) | ||
| val primaryLocaleSource = LocaleSource.createFromPrimary(localeContext) | ||
| val fallbackLocaleSource = LocaleSource.createFromFallback(localeContext) | ||
| val proposal = chooser.findBestProposal(primaryLocaleSource, fallbackLocaleSource) | ||
| memoizedLocales.putIfAbsent(localeContext, proposal.computedLocale) | ||
| proposal.computedLocale | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here & elsewhere: could we use Kotlin's https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/get-or-put.html instead? That would avoid the SDK-specific API that we're using here (and avoid needing to have two different implementations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion ,
I think its enough to use getOrPut combined with synchronized function of kotlin to avoid unnecessary computation , as the main use of computeIfAbsent seemingly is to avoid unnecessary computation.
I think we can replace if condition by using code in the below format :
instead of writing like :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed that getOrPut doesn't allow null value to be returned by defaultValue function ,
so in places where it is possible to receive null after computing for the key , we can use ?.let scope and putIfAbsent to account for NullPointerException.
Have implemented it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I want to try and avoid the synchronized bit if we can since it's pretty highly discouraged in Kotlin (see https://kt.academy/article/ek-synchronization) since it can cause problems with coroutines (see https://blog.danlew.net/2020/01/28/coroutines-and-java-synchronization-dont-mix/).
That being said, getOrPut does not guarantee atomicity, unlike does computeIfAbsent for ConcurrentHashMap. This may actually be a case where a slight change in implementation is needed to better leverage a lockless approach via coroutines. I'll follow up in a second comment with suggested versions of this & the next class to try and better illustrate what I'm thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I'm thinking for fixing the issue with locale creation: https://gist.github.com/BenHenning/deb77757e89e1974066a6e76d1496f8b/revisions. There will be some peripheral changes needed in other files, too.
For FakeAssetRepository, we ideally would update it in the same way (i.e. update AssetRepository to returned
Deferreds instead, which would also let us get rid of the ReentrantLock AssetRepositoryImpl is currently using). However, that's a change that will affect a lot more in the codebase, and doesn't seem worth it for the immediate benefit of fixing the crash. Could we maybe instead copy the ReentrantLock pattern used in the production implementation (https://github.com/oppia/oppia-android/blob/develop/utility/src/main/java/org/oppia/android/util/caching/AssetRepositoryImpl.kt)? It has the same problem as synchronized inherently, though it offers better consistency with other locks used in the codebase and offers a variety of benefits over the built-in JVM lock (see https://stackoverflow.com/a/11821900).