Fix for LRU cache inconsistency crash #7836

cafesilencio · 2024-06-25T17:37:28Z

Description

In response to: https://mapbox.atlassian.net/browse/NAVAND-3208

Put call to cache route data on it's own thread so the synchronize blocks in the caching class will be effective. The caching call is in a method that is being called from a coroutine.

Screenshots or Gifs

github-actions · 2024-06-25T17:37:53Z

Changelog

Features

Bug fixes and improvements

Fixed UI jank caused by on-device TextToSpeech player. [#7833](https://github.com/mapbox/mapbox-navigation-android/pull/7833)

Known issues ⚠️

Other changes

Android Auto Changelog

Features

Bug fixes and improvements

The app is now considered as the one in active navigation only when an active route is set to MapboxNavigation. Previously it was always considered active. [#7366](https://github.com/mapbox/mapbox-navigation-android/pull/7366)
When Android Auto host tells the app to stop active navigation because another app starts navigating, now SDK will enter FreeDrive mode instead of stopping trip session completely. [#7366](https://github.com/mapbox/mapbox-navigation-android/pull/7366)

codecov · 2024-06-25T17:53:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.18%. Comparing base (9b40acb) to head (286310c).

@@            Coverage Diff            @@
##               main    #7836   +/-   ##
=========================================
  Coverage     74.17%   74.18%           
  Complexity     6262     6262           
=========================================
  Files           856      856           
  Lines         33789    33793    +4     
  Branches       4023     4022    -1     
=========================================
+ Hits          25062    25068    +6     
+ Misses         7170     7169    -1     
+ Partials       1557     1556    -1

Files	Coverage Δ
...ion/base/internal/route/RouteCompatibilityCache.kt	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

dmitry-novikov · 2024-07-02T07:52:53Z

...tion-base/src/main/java/com/mapbox/navigation/base/internal/route/RouteCompatibilityCache.kt

@@ -17,37 +18,51 @@ import com.mapbox.navigation.base.route.NavigationRoute
 object RouteCompatibilityCache {
    private val directionsSessionCache = mutableListOf<NavigationRoute>()
    private val creationCache = LruCache<DirectionsRoute, NavigationRoute>(3)
-    private val lock = Any()
+    private val semaphore = Semaphore(1)


@cafesilencio, can you elaborate on how this is fixing the issue?

I can't recreate the issue but I've seen discussions about synchronized blocks not working well with coroutines. Based on the stack trace something isn't correct with using a generic object to lock on to. I thought using a Semaphore would be a stronger locking mechanism in this case. Do you have a suggestion for something different?

I think both mechanism are the same – they both will lock current thread until lock/semaphore is free. So looks like this fix will improve nothing, IMO.

if we need to sync it with coroutines, we can make the func suspend and use coroutine specific locks:
'ReentrantLock' / Mutex

it also can be a Semaphore but from kotlinx.coroutines.sync

we can't use classes from java.util.concurrent with coroutines

we can't use classes from java.util.concurrent with coroutines

Thanks for the clarification. I knew there was some kind of incompatibility but I couldn't remember the details.

we can't use classes from java.util.concurrent with coroutines

There is no such limitation, where did you get that?

@korshaknn might know the details better and can comment. I seem to remember that synchronize doesn't work with coroutines because they can operate on the same thread. If you look at the the stack trace in the reported error it seems evident the synchronize blocks aren't working. There is one call to this class originating in a coroutine.

Coroutines provide better alternatives, but synchronization still works

I mean java.util.concurrent with coroutines will block the whole thread, that's why we should use coroutine specific sync classes
Mutex.lock() is a suspending function. It does not block a thread.

Doesn't mean we can't use synchronization with coroutines or that it doesn't work

cafesilencio · 2024-07-02T22:32:15Z

Some additional context:

RouteCompatibilityCache::setDirectionsSessionResult is called in MapboxDirectionsSession::setNavigationRoutesFinished which is executed in a coroutine in MapboxNavigation::internalSetNavigationRoutes line 1133. The other functions in RouteCompatibilityCache are not called in a coroutine as far as I can see.

Zayankovsky · 2024-07-08T20:44:00Z

...tion-base/src/main/java/com/mapbox/navigation/base/internal/route/RouteCompatibilityCache.kt

+        // A crash was reported due to an LruCache inconsistency.
+        // The caching implementation is using java.concurrent
+        // classes for synchronization. This method is getting called
+        // from a coroutine upstream. Putting this call on it's own thread
+        // should make the synchronization effective.
+        Thread {
+            run {
+                setDirectionsSessionResultInternal(routes)
+            }
+        }.start()


I really doubt that changes anything

My thinking was that since synchronize is designed to work with threads rather than coroutines I could execute this code on a new thread and the synchronization would be enforced. If I change the class to use suspend methods and a kotlin mutex internally that will cause the upstream usage to change. The methods in this class are ultimately called in MapboxNavigation. If you have suggestions please share them. Based on the stack trace reported, the synchronization blocks seem not to be working correctly. The only explanation I could think of was that one of the methods in this class is originating from a coroutine upstream.

Any coroutine code is still run on some thread and synchronization works there as usual. The only thing you can't do is call suspend functions inside synchronized blocks, but that is checked during compilation

For me this looks like an issue with hashCode or equals, which causes the LruCache to run into issues

Another option could be to use single thread for all the operations and remove synchronisation at all.
Like here.

Any coroutine code is still run on some thread and synchronization works there as usual. The only thing you can't do is call suspend functions inside synchronized blocks, but that is checked during compilation

You might be right but several coroutines can be associated with the same thread. So I thought if I put the operation on an actual new thread the synchronization would be more effective.

For me this looks like an issue with hashCode or equals, which causes the LruCache to run into issues

Here is the stack trace from the reported issue. I don't think this is a result of a haschCode/equals problem.

IllegalStateException
android.util.LruCache.sizeOf() is reporting inconsistent results!

android.util.LruCache in trimToSize at line 203
android.util.LruCache in evictAll at line 311
com.mapbox.navigation.base.internal.route.RouteCompatibilityCache in setDirectionsSessionResult at line 38
com.mapbox.navigation.core.directions.session.MapboxDirectionsSession in setNavigationRoutesFinished at line 49
com.mapbox.navigation.core.MapboxNavigation$internalSetNavigationRoutes$2 in invokeSuspend at line 1129
kotlin.coroutines.jvm.internal.BaseContinuationImpl in resumeWith at line 33
kotlinx.coroutines.internal.DispatchedContinuation in resumeUndispatchedWith$kotlinx_coroutines_core at line 256
kotlinx.coroutines.DispatchedTaskKt in resume at line 178
kotlinx.coroutines.DispatchedTaskKt in resumeUnconfined at line 191
kotlinx.coroutines.DispatchedTaskKt in dispatch at line 163
kotlinx.coroutines.CancellableContinuationImpl in dispatchResume at line 474
kotlinx.coroutines.CancellableContinuationImpl in resumeImpl at line 508
kotlinx.coroutines.CancellableContinuationImpl in resumeImpl$default at line 497
kotlinx.coroutines.CancellableContinuationImpl in resumeWith at line 368
com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl$refreshRoute$callback$1 in invoke$lambda-2 at line 267
com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl$refreshRoute$callback$1 in $r8$lambda$S3_XwtbgUvIeW_lYTzEbEd0UjcE
… $1$$InternalSyntheticLambda$3$5ce679cfe3c67248c45a37495894744b0b268c2244e2f7d1b83487200930d94f$1 in invoke
com.mapbox.bindgen.Expected in fold at line 81
com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl$refreshRoute$callback$1 in invoke at line 248
com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl$refreshRoute$callback$1 in invoke at line 245
com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl$refreshRoute$2$1 in run at line 282
android.os.MessageQueue in nativePollOnce
android.os.MessageQueue in next at line 342
android.os.Looper in loopOnce at line 182
android.os.Looper in loop at line 357
android.app.ActivityThread in main at line 8194
java.lang.reflect.Method in invoke
com.android.internal.os.RuntimeInit$MethodAndArgsCaller in run at line 548
com.android.internal.os.ZygoteInit in main at line 957

Another option could be to use single thread for all the operations and remove synchronisation at all.

I can look at doing something like that but I can't change the methods in the caching class to suspend methods because upstream the caching class is used MapboxNavigation and would require changing some of those methods to suspend which would break SEMVER.

cafesilencio · 2024-07-09T20:06:24Z

I replaced the LRUCache with a ConcurrentHashMap and resize as necessary. Please take a look.

Zayankovsky

Well, at least it won't crash anymore, since ConcurrentHashMap doesn't do this kind of consistency checks

…7843) Co-authored-by: Seth Bourget <[email protected]>

cafesilencio added the skip changelog Should not be added into version changelog. label Jun 25, 2024

cafesilencio requested a review from a team as a code owner June 25, 2024 17:37

cafesilencio marked this pull request as draft June 25, 2024 19:10

cafesilencio force-pushed the sb-NAVAND-3208-sync-lru-cache branch from 77025d3 to a89e1c4 Compare June 25, 2024 19:21

cafesilencio marked this pull request as ready for review June 25, 2024 19:22

cafesilencio changed the title ~~Changed synchronization object based on documentation in LruCache class.~~ Changed synchronization to semaphore. Jun 26, 2024

cafesilencio requested a review from a team July 1, 2024 16:51

dmitry-novikov reviewed Jul 2, 2024

View reviewed changes

cafesilencio force-pushed the sb-NAVAND-3208-sync-lru-cache branch 2 times, most recently from 6bebfdd to 3643e05 Compare July 3, 2024 20:35

cafesilencio changed the title ~~Changed synchronization to semaphore.~~ Fix for LRU cache inconsistency crash Jul 8, 2024

cafesilencio force-pushed the sb-NAVAND-3208-sync-lru-cache branch 7 times, most recently from fd57277 to a840c74 Compare July 8, 2024 19:49

cafesilencio requested a review from korshaknn July 8, 2024 20:15

Zayankovsky reviewed Jul 8, 2024

View reviewed changes

Using semaphore rather than synchronizing on generic object.

286310c

cafesilencio force-pushed the sb-NAVAND-3208-sync-lru-cache branch from a840c74 to 286310c Compare July 9, 2024 20:03

Zayankovsky approved these changes Jul 10, 2024

View reviewed changes

cafesilencio merged commit ae64202 into main Jul 10, 2024
39 of 43 checks passed

cafesilencio deleted the sb-NAVAND-3208-sync-lru-cache branch July 10, 2024 16:41

DzmitryFomchyn pushed a commit that referenced this pull request Jul 22, 2024

Using semaphore rather than synchronizing on generic object. (#7836)

8790e50

DzmitryFomchyn mentioned this pull request Jul 22, 2024

Fix for LRU cache inconsistency crash cherry-pick #7843

Merged

DzmitryFomchyn added a commit that referenced this pull request Jul 22, 2024

Using semaphore rather than synchronizing on generic object. (#7836) (#…

de7281c

…7843) Co-authored-by: Seth Bourget <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for LRU cache inconsistency crash #7836

Fix for LRU cache inconsistency crash #7836

cafesilencio commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

codecov bot commented Jun 25, 2024 •

edited

Loading

dmitry-novikov Jul 2, 2024

cafesilencio Jul 2, 2024

dmitry-novikov Jul 2, 2024

korshaknn Jul 2, 2024 •

edited

Loading

cafesilencio Jul 2, 2024

Zayankovsky Jul 8, 2024

cafesilencio Jul 8, 2024 •

edited

Loading

Zayankovsky Jul 9, 2024

korshaknn Jul 10, 2024

Zayankovsky Jul 10, 2024 •

edited

Loading

cafesilencio commented Jul 2, 2024

Zayankovsky Jul 8, 2024

cafesilencio Jul 8, 2024

Zayankovsky Jul 9, 2024

Zayankovsky Jul 9, 2024

dmitry-novikov Jul 9, 2024

cafesilencio Jul 9, 2024

cafesilencio commented Jul 9, 2024

Zayankovsky left a comment

Fix for LRU cache inconsistency crash #7836

Fix for LRU cache inconsistency crash #7836

Conversation

cafesilencio commented Jun 25, 2024 • edited Loading

Description

Screenshots or Gifs

github-actions bot commented Jun 25, 2024

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

codecov bot commented Jun 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korshaknn Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cafesilencio Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zayankovsky Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

cafesilencio commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cafesilencio commented Jul 9, 2024

Zayankovsky left a comment

Choose a reason for hiding this comment

cafesilencio commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading

korshaknn Jul 2, 2024 •

edited

Loading

cafesilencio Jul 8, 2024 •

edited

Loading

Zayankovsky Jul 10, 2024 •

edited

Loading