Fix race conditions on SQLiteScoreList

iSoron · Jun 10, 2017 · fe1513b · fe1513b
1 parent e06ace9
commit fe1513b
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 27 deletions.
diff --git a/app/src/androidTest/java/org/isoron/uhabits/BaseAndroidTest.java b/app/src/androidTest/java/org/isoron/uhabits/BaseAndroidTest.java
@@ -67,6 +67,8 @@ public class BaseAndroidTest
 
     protected ModelFactory modelFactory;
 
+    private boolean isDone = false;
+
     @Before
     public void setUp()
     {
@@ -117,6 +119,25 @@ protected void awaitLatch() throws InterruptedException
         assertTrue(latch.await(60, TimeUnit.SECONDS));
     }
 
+    protected void runConcurrently(Runnable... runnableList) throws Exception
+    {
+        isDone = false;
+        ExecutorService executor = Executors.newFixedThreadPool(100);
+        List<Future> futures = new LinkedList<>();
+        for (Runnable r : runnableList)
+            futures.add(executor.submit(() ->
+            {
+                while (!isDone) r.run();
+                return null;
+            }));
+
+        Thread.sleep(3000);
+        isDone = true;
+        executor.shutdown();
+        for(Future f : futures) f.get();
+        while (!executor.isTerminated()) Thread.sleep(50);
+    }
+
     protected void setTheme(@StyleRes int themeId)
     {
         targetContext.setTheme(themeId);

diff --git a/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteScoreListTest.java b/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteScoreListTest.java
@@ -61,29 +61,6 @@ public void setUp()
         day = DateUtils.millisecondsInOneDay;
     }
 
-    @Test
-    public void testGetAll()
-    {
-        List<Score> list = scores.toList();
-        assertThat(list.size(), equalTo(121));
-        assertThat(list.get(0).getTimestamp(), equalTo(today));
-        assertThat(list.get(10).getTimestamp(), equalTo(today - 10 * day));
-    }
-
-    @Test
-    public void testInvalidateNewerThan()
-    {
-        scores.getTodayValue(); // force recompute
-        List<ScoreRecord> records = getAllRecords();
-        assertThat(records.size(), equalTo(121));
-
-        scores.invalidateNewerThan(today - 10 * day);
-
-        records = getAllRecords();
-        assertThat(records.size(), equalTo(110));
-        assertThat(records.get(0).timestamp, equalTo(today - 11 * day));
-    }
-
     @Test
     public void testAdd()
     {
@@ -101,6 +78,15 @@ public void testAdd()
         assertThat(records.get(0).timestamp, equalTo(today));
     }
 
+    @Test
+    public void testGetAll()
+    {
+        List<Score> list = scores.toList();
+        assertThat(list.size(), equalTo(121));
+        assertThat(list.get(0).getTimestamp(), equalTo(today));
+        assertThat(list.get(10).getTimestamp(), equalTo(today - 10 * day));
+    }
+
     @Test
     public void testGetByInterval()
     {
@@ -115,6 +101,16 @@ public void testGetByInterval()
         assertThat(list.get(7).getTimestamp(), equalTo(today - 10 * day));
     }
 
+    @Test
+    public void testGetByInterval_concurrent() throws Exception
+    {
+        Runnable block1 = () -> scores.invalidateNewerThan(0);
+        Runnable block2 =
+            () -> assertThat(scores.getByInterval(today, today).size(),
+                equalTo(1));
+        runConcurrently(block1, block2);
+    }
+
     @Test
     public void testGetByInterval_withLongInterval()
     {
@@ -125,6 +121,30 @@ public void testGetByInterval_withLongInterval()
         assertThat(list.size(), equalTo(201));
     }
 
+    @Test
+    public void testGetTodayValue_concurrent() throws Exception
+    {
+        Runnable block1 = () -> scores.invalidateNewerThan(0);
+        Runnable block2 =
+            () -> assertThat(scores.getTodayValue(), equalTo(18407827));
+
+        runConcurrently(block1, block2);
+    }
+
+    @Test
+    public void testInvalidateNewerThan()
+    {
+        scores.getTodayValue(); // force recompute
+        List<ScoreRecord> records = getAllRecords();
+        assertThat(records.size(), equalTo(121));
+
+        scores.invalidateNewerThan(today - 10 * day);
+
+        records = getAllRecords();
+        assertThat(records.size(), equalTo(110));
+        assertThat(records.get(0).timestamp, equalTo(today - 11 * day));
+    }
+
     private List<ScoreRecord> getAllRecords()
     {
         return new Select()

diff --git a/app/src/main/java/org/isoron/uhabits/models/ScoreList.java b/app/src/main/java/org/isoron/uhabits/models/ScoreList.java
@@ -81,7 +81,7 @@ public int getTodayValue()
      * @param timestamp the timestamp of a day
      * @return score value for that day
      */
-    public final synchronized int getValue(long timestamp)
+    public synchronized final int getValue(long timestamp)
     {
         compute(timestamp, timestamp);
         Score s = getComputedByTimestamp(timestamp);

diff --git a/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteScoreList.java b/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteScoreList.java
@@ -100,7 +100,8 @@ public void add(List<Score> scores)
 
     @NonNull
     @Override
-    public List<Score> getByInterval(long fromTimestamp, long toTimestamp)
+    public synchronized List<Score> getByInterval(long fromTimestamp,
+                                                  long toTimestamp)
     {
         check(habit.getId());
         compute(fromTimestamp, toTimestamp);
@@ -137,7 +138,7 @@ public Score getComputedByTimestamp(long timestamp)
     }
 
     @Override
-    public int getTodayValue()
+    public synchronized int getTodayValue()
     {
         if (cache == null || cache.expired())
             cache = new CachedData(super.getTodayValue());
@@ -146,7 +147,7 @@ public int getTodayValue()
     }
 
     @Override
-    public void invalidateNewerThan(long timestamp)
+    public synchronized void invalidateNewerThan(long timestamp)
     {
         cache = null;
         invalidateStatement.bindLong(1, habit.getId());