Skip to content
This repository has been archived by the owner on Jul 2, 2021. It is now read-only.

Fix concurrency bug in Replay #43

Merged
merged 4 commits into from Oct 10, 2019
Merged

Conversation

jacksoncheek
Copy link
Contributor

JIRA

https://compass-tech.atlassian.net/browse/CEI-504

Summary

  • Replay has a concurrency bug. When multiple subscribers are posting values to it from different threads, we can hit an index out of bounds exception.
  • This is causing a semi-major bug in the offline banner feature on Android.
  • Adds a test (that breaks without the changes) and fixes Replay by adding concurrency locks around the internal values list.

Stacktrace

Fatal Exception: java.lang.ArrayIndexOutOfBoundsException: length=2; index=2
       at java.util.ArrayList.add(ArrayList.java:468)
       at com.compass.snail.Replay.next(Replay.kt:17)
       at com.compass.snail.Variable.setValue(Variable.kt:18)
       at com.compass.compasslibrary.services.NetworkBroadcastReceiverService.updateConnection(NetworkBroadcastReceiverService.kt:42)
       at com.compass.compasslibrary.apiv3.ModelRequest$request$1$2.onFailure(ModelRequest.kt:59)
       at com.google.firebase.perf.network.zzf.onFailure(com.google.firebase:firebase-perf@@18.0.1:18)
       at okhttp3.RealCall$AsyncCall.execute(RealCall.java:161)
       at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
       at java.lang.Thread.run(Thread.java:764)

@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #43 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   86.93%   87.15%   +0.21%     
==========================================
  Files          17       17              
  Lines         176      179       +3     
  Branches       14       14              
==========================================
+ Hits          153      156       +3     
  Misses         18       18              
  Partials        5        5
Impacted Files Coverage Δ
...l-kotlin/src/main/java/com/compass/snail/Replay.kt 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89cca75...3d51773. Read the comment docs.

@jacksoncheek
Copy link
Contributor Author

Reverting from queue data structure, our Rx framework supports null items and the thread-safe LinkedBlockingQueue in the Java concurrent util package does not.

@russellbstephens russellbstephens merged commit f6b15c4 into master Oct 10, 2019
@russellbstephens russellbstephens deleted the fix-replay-concurrency-bug branch October 10, 2019 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants