fix: algorithm concurrency #761
Conversation
Ensure methods operate on the same algorithm reference during lock, operation, and unlock. Ensure algorithm reference is not replaced while in use.
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #761 +/- ##
==========================================
- Coverage 39.20% 39.09% -0.12%
==========================================
Files 71 71
Lines 4076 4088 +12
Branches 609 609
==========================================
Hits 1598 1598
- Misses 2375 2387 +12
Partials 103 103
Continue to review full report at Codecov.
|
| @@ -79,6 +81,7 @@ | |||
| private final ClusterManager<T> mClusterManager; | |||
| private final float mDensity; | |||
| private boolean mAnimate; | |||
| private final Executor mExecutor = Executors.newSingleThreadExecutor(); | |||
There was a problem hiding this comment.
Any reason this should be single threaded vs. a larger thread pool? Making this single threaded changes the current behavior/performance.
There was a problem hiding this comment.
Yes, as noted by @barbeau #601 (comment) when reviewing the original PR, the implementation is effectively single threaded already, which is why I chose to use a single thread executor. There's only ever one render task in flight and potentially one queued up to run next.
There was a problem hiding this comment.
Got it! That wasn't immediately obvious to me so this is definitely a nice improvement 👍
| @@ -156,11 +162,12 @@ public void setAnimation(boolean animate) { | |||
| * {@link #cluster()} for the map to be cleared. | |||
| */ | |||
| public void clearItems() { | |||
| mAlgorithm.lock(); | |||
| Algorithm<T> algorithm = getAlgorithm(); | |||
There was a problem hiding this comment.
Should these local declarations of Algorithm be declared final, to be safe?
There was a problem hiding this comment.
Yeah, good call. That makes it more explicit they should remain constant through the entire method execution.
## [2.0.2](v2.0.1...v2.0.2) (2020-07-07) ### Bug Fixes * algorithm concurrency ([#761](#761)) ([bad7771](bad7771))
Lock and unlock a local algorithm reference. Ensure methods operate on the same algorithm reference during lock, operation, and unlock. Ensure algorithm reference is not replaced while in use.
This crash is caused by calling
ClusterManager.setAlgorithm()immediately afterClusterManager.cluster()such that themAlgorithmreference is changed in the middle of the background cluster task. SomAlgorithm.lock()andmAlgorithm.unlock()are actually referencing first the old and then the new algorithm. The thread that calledsetAlgorithm()is the one that locks the algorithm and theAsyncTaskbackground thread is the one that attempts to unlock it, resulting in the crash.This bug was introduced in #506 when the algorithm lock was moved from the
ClusterManagerto theAlgorithm, which allows the algorithm to be accessed by aViewModel, outside theClusterManager, which can be recreated as part of the view layer.Also restores #601 to use thread pools, since this wasn't the cause for this bug.
Fixes #660 🦕