Repository: aurora Updated Branches: refs/heads/master 7e32c4381 -> 8f98e6e8a
Do not modify list populated by MyBatis. A ConcurrentModificationException can occur when multiple threads are sorting the same task event list. This is possible because we have enabled MyBatis caching with the readOnly flag which can use the same instance to populate different `DbScheduledTask` instances. Bugs closed: AURORA-1625 Reviewed at https://reviews.apache.org/r/44106/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/8f98e6e8 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/8f98e6e8 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/8f98e6e8 Branch: refs/heads/master Commit: 8f98e6e8a50e59abfd4b80a0aa2696a6bed6daf9 Parents: 7e32c43 Author: Zameer Manji <[email protected]> Authored: Fri Feb 26 15:58:01 2016 -0800 Committer: Zameer Manji <[email protected]> Committed: Fri Feb 26 15:58:08 2016 -0800 ---------------------------------------------------------------------- .../storage/db/views/DbScheduledTask.java | 6 ++-- .../storage/AbstractTaskStoreTest.java | 29 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/8f98e6e8/src/main/java/org/apache/aurora/scheduler/storage/db/views/DbScheduledTask.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/storage/db/views/DbScheduledTask.java b/src/main/java/org/apache/aurora/scheduler/storage/db/views/DbScheduledTask.java index 8d0dab0..6260923 100644 --- a/src/main/java/org/apache/aurora/scheduler/storage/db/views/DbScheduledTask.java +++ b/src/main/java/org/apache/aurora/scheduler/storage/db/views/DbScheduledTask.java @@ -13,7 +13,6 @@ */ package org.apache.aurora.scheduler.storage.db.views; -import java.util.Collections; import java.util.List; import com.google.common.collect.Ordering; @@ -35,13 +34,14 @@ public final class DbScheduledTask { } public IScheduledTask toImmutable() { - Collections.sort(taskEvents, BY_TIMESTAMP); return IScheduledTask.build( new ScheduledTask() .setAssignedTask(assignedTask.toThrift()) .setStatus(status) .setFailureCount(failureCount) - .setTaskEvents(taskEvents) + // Must be sorting a copy because taskEvents is populated by MyBatis and it might + // reuse the same instance. + .setTaskEvents(BY_TIMESTAMP.immutableSortedCopy(taskEvents)) .setAncestorId(ancestorId)); } http://git-wip-us.apache.org/repos/asf/aurora/blob/8f98e6e8/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java b/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java index dc5e2bd..e56fed2 100644 --- a/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java +++ b/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java @@ -13,11 +13,13 @@ */ package org.apache.aurora.scheduler.storage; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import com.google.common.base.Optional; @@ -25,7 +27,12 @@ import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.inject.Guice; @@ -552,6 +559,28 @@ public abstract class AbstractTaskStoreTest extends TearDownTestCase { saveTasks(c); } + @Test + public void testConcurrentFetchTasks() throws Exception { + // Test for regression of AURORA-1625 + ListeningExecutorService executor = + MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(5)); + assertStoreContents(); + saveTasks(TASK_A, TASK_B, TASK_C, TASK_D); + + List<ListenableFuture<Integer>> futures = Lists.newArrayList(); + + for (int i = 0; i < 100; i++) { + futures.add(executor.submit(() -> Iterables.size(fetchTasks(Query.unscoped())))); + } + + Future<List<Integer>> f = Futures.allAsList(futures); + + executor.shutdown(); + executor.awaitTermination(1, TimeUnit.MINUTES); + + assertEquals(Iterables.getOnlyElement(ImmutableSet.copyOf(f.get())), (Integer) 4); + } + private void assertStoreContents(IScheduledTask... tasks) { assertQueryResults(Query.unscoped(), tasks); }
