From 49d868b491b5057d6f79914c028e77fb6d6c3e34 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Thu, 24 Mar 2016 16:30:34 +0100 Subject: [PATCH 1/2] Improve test to not rely on thread slowness We have to swap the second latch before we count it down otherwise threads might be faster than the test. This has happend on a recent CI failure: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob-os-compatibility/os=ubuntu/121/console This commit also adds a synchronized on the close method since it's canceling and modifying a member varialbe that is assigned under lock. --- .../org/elasticsearch/index/IndexService.java | 2 +- .../elasticsearch/index/IndexServiceTests.java | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/IndexService.java b/core/src/main/java/org/elasticsearch/index/IndexService.java index b2c218ae10d..62076701183 100644 --- a/core/src/main/java/org/elasticsearch/index/IndexService.java +++ b/core/src/main/java/org/elasticsearch/index/IndexService.java @@ -780,7 +780,7 @@ public final class IndexService extends AbstractIndexComponent implements IndexC } @Override - public void close() { + public synchronized void close() { if (closed.compareAndSet(false, true)) { FutureUtils.cancel(scheduledFuture); scheduledFuture = null; diff --git a/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java b/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java index 3a4020e4103..2e4b726d0a0 100644 --- a/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java +++ b/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.concurrent.CountDown; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; @@ -42,6 +43,7 @@ import java.io.IOException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BiFunction; import static org.elasticsearch.index.query.QueryBuilders.termQuery; import static org.hamcrest.Matchers.containsString; @@ -187,11 +189,18 @@ public class IndexServiceTests extends ESSingleNodeTestCase { return ThreadPool.Names.GENERIC; } }; + + BiFunction, CountDownLatch, CountDownLatch> swapAndReturn = (ref, newLatch) -> { + CountDownLatch downLatch = ref.get(); + ref.set(newLatch); + return downLatch; + }; latch.get().await(); - latch.set(new CountDownLatch(1)); + swapAndReturn.apply(latch, new CountDownLatch(1)); assertEquals(1, count.get()); - latch2.get().countDown(); - latch2.set(new CountDownLatch(1)); + // here we need to swap first before we let it go otherwise threads might be very fast and run that task twice due to + // random exception and the schedule interval is 1ms + swapAndReturn.apply(latch2, new CountDownLatch(1)).countDown(); latch.get().await(); assertEquals(2, count.get()); From f81fb89ea545c518bd5d5d1a78397bd391c04099 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Thu, 24 Mar 2016 16:48:50 +0100 Subject: [PATCH 2/2] simplify test to not use lambda at all --- .../org/elasticsearch/index/IndexServiceTests.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java b/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java index 2e4b726d0a0..97258b12a3b 100644 --- a/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java +++ b/core/src/test/java/org/elasticsearch/index/IndexServiceTests.java @@ -27,7 +27,6 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.util.concurrent.CountDown; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; @@ -43,7 +42,6 @@ import java.io.IOException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; -import java.util.function.BiFunction; import static org.elasticsearch.index.query.QueryBuilders.termQuery; import static org.hamcrest.Matchers.containsString; @@ -190,18 +188,12 @@ public class IndexServiceTests extends ESSingleNodeTestCase { } }; - BiFunction, CountDownLatch, CountDownLatch> swapAndReturn = (ref, newLatch) -> { - CountDownLatch downLatch = ref.get(); - ref.set(newLatch); - return downLatch; - }; latch.get().await(); - swapAndReturn.apply(latch, new CountDownLatch(1)); + latch.set(new CountDownLatch(1)); assertEquals(1, count.get()); // here we need to swap first before we let it go otherwise threads might be very fast and run that task twice due to // random exception and the schedule interval is 1ms - swapAndReturn.apply(latch2, new CountDownLatch(1)).countDown(); - + latch2.getAndSet(new CountDownLatch(1)).countDown(); latch.get().await(); assertEquals(2, count.get()); task.close();