From a39b21b71f718b98a8a678b702a67a274a065e91 Mon Sep 17 00:00:00 2001 From: Mario Catalan Date: Fri, 5 Mar 2021 15:34:10 +0100 Subject: [PATCH 1/4] Fix for https://github.com/perwendel/spark/issues/1118 --- src/main/java/spark/route/Routes.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/spark/route/Routes.java b/src/main/java/spark/route/Routes.java index 518d050314..11d2d4c2a2 100644 --- a/src/main/java/spark/route/Routes.java +++ b/src/main/java/spark/route/Routes.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -49,7 +50,7 @@ public static Routes create() { * Constructor */ protected Routes() { - routes = new ArrayList<>(); + routes = new CopyOnWriteArrayList<>(); } /** From 063c0532c4e03ec58efd407bd12f5afab3340cd1 Mon Sep 17 00:00:00 2001 From: Mario Catalan Date: Tue, 9 Mar 2021 02:34:52 +0100 Subject: [PATCH 2/4] Added concurrency test for class Routes --- src/main/java/spark/FilterImpl.java | 4 +- src/main/java/spark/RouteImpl.java | 2 +- .../spark/route/RoutesConcurrencyTest.java | 99 +++++++++++++++++++ 3 files changed, 102 insertions(+), 3 deletions(-) create mode 100644 src/test/java/spark/route/RoutesConcurrencyTest.java diff --git a/src/main/java/spark/FilterImpl.java b/src/main/java/spark/FilterImpl.java index a138fd6726..2a5b29115f 100644 --- a/src/main/java/spark/FilterImpl.java +++ b/src/main/java/spark/FilterImpl.java @@ -27,7 +27,7 @@ */ public abstract class FilterImpl implements Filter, Wrapper { - static final String DEFAULT_ACCEPT_TYPE = "*/*"; + public static final String DEFAULT_ACCEPT_TYPE = "*/*"; private String path; private String acceptType; @@ -63,7 +63,7 @@ static FilterImpl create(final String path, final Filter filter) { * @param filter the filter * @return the wrapped route */ - static FilterImpl create(final String path, String acceptType, final Filter filter) { + public static FilterImpl create(final String path, String acceptType, final Filter filter) { if (acceptType == null) { acceptType = DEFAULT_ACCEPT_TYPE; } diff --git a/src/main/java/spark/RouteImpl.java b/src/main/java/spark/RouteImpl.java index 543dfefc15..1f102d23a1 100644 --- a/src/main/java/spark/RouteImpl.java +++ b/src/main/java/spark/RouteImpl.java @@ -26,7 +26,7 @@ * @author Per Wendel */ public abstract class RouteImpl implements Route, Wrapper { - static final String DEFAULT_ACCEPT_TYPE = "*/*"; + public static final String DEFAULT_ACCEPT_TYPE = "*/*"; private String path; private String acceptType; diff --git a/src/test/java/spark/route/RoutesConcurrencyTest.java b/src/test/java/spark/route/RoutesConcurrencyTest.java new file mode 100644 index 0000000000..58ffafdf2c --- /dev/null +++ b/src/test/java/spark/route/RoutesConcurrencyTest.java @@ -0,0 +1,99 @@ +package spark.route; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import spark.FilterImpl; +import spark.RouteImpl; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class RoutesConcurrencyTest { + private final AtomicInteger numberOfSuccessfulIterations = new AtomicInteger(); + private final AtomicInteger numberOfFailedIterations = new AtomicInteger(); + private ExecutorService executorService; + + private static final int NUMBER_OF_ITERATIONS = 10_000; + private static final int NUMBER_OF_THREADS = 2; + private static final int NUMBER_OF_TASKS = NUMBER_OF_THREADS; + + private static final String ROUTE_PATH_PREFIX = "/route/"; + private static final String FILTER_PATH_PREFIX = "/filter/"; + + @Before + public void setup() { + numberOfSuccessfulIterations.set(0); + numberOfFailedIterations.set(0); + } + + @After + public void teardown() { + if (executorService != null && !executorService.isShutdown()) { + try { + executorService.shutdownNow(); + if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) { + System.err.println("Executor service did not terminate."); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + } + + @Test + public void classShouldBeThreadSafe() throws Exception { + executorService = Executors.newFixedThreadPool(NUMBER_OF_THREADS); + Collection> tasks = partitionIterationsIntoTasks(); + List> futureResults = executorService.invokeAll(tasks); + executorService.shutdown(); + for (Future futureResult : futureResults) { + futureResult.get(); + assertTrue(futureResult.isDone()); + } + assertEquals(NUMBER_OF_ITERATIONS, numberOfSuccessfulIterations.intValue()); + assertEquals(0, numberOfFailedIterations.intValue()); + } + + private Collection> partitionIterationsIntoTasks() { + final Collection> tasks = new ArrayList<>(); + final Routes routes = Routes.create(); + final int numberOfIterationsPerTask = NUMBER_OF_ITERATIONS / NUMBER_OF_TASKS; + for (int taskIndex = 0; taskIndex < NUMBER_OF_TASKS; taskIndex++) { + final int fromIteration = numberOfIterationsPerTask * taskIndex; + final int toIteration = numberOfIterationsPerTask * (taskIndex + 1); + tasks.add(() -> { + for (int iterationIndex = fromIteration; iterationIndex < toIteration; iterationIndex++) { + try { + String routePath = ROUTE_PATH_PREFIX + iterationIndex; + String filterPath = FILTER_PATH_PREFIX + iterationIndex; + routes.add(HttpMethod.get, RouteImpl.create(routePath, RouteImpl.DEFAULT_ACCEPT_TYPE, null)); + routes.add(HttpMethod.get, FilterImpl.create(filterPath, FilterImpl.DEFAULT_ACCEPT_TYPE, null)); + routes.find(HttpMethod.get, routePath, RouteImpl.DEFAULT_ACCEPT_TYPE); + routes.findMultiple(HttpMethod.get, filterPath, FilterImpl.DEFAULT_ACCEPT_TYPE); + routes.findAll(); + routes.remove(routePath, HttpMethod.get.toString()); + routes.remove(filterPath); + routes.clear(); + numberOfSuccessfulIterations.getAndIncrement(); + } catch (Exception e) { + numberOfFailedIterations.getAndIncrement(); + } + } + return null; + }); + } + return tasks; + } +} From 06d83272f6cf56001e00839b52f927c045bc883b Mon Sep 17 00:00:00 2001 From: Mario Catalan Date: Tue, 9 Mar 2021 16:21:25 +0100 Subject: [PATCH 3/4] Minor improvements. --- src/main/java/spark/route/Routes.java | 2 +- .../spark/route/RoutesConcurrencyTest.java | 22 +++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/main/java/spark/route/Routes.java b/src/main/java/spark/route/Routes.java index 11d2d4c2a2..2478c06b74 100644 --- a/src/main/java/spark/route/Routes.java +++ b/src/main/java/spark/route/Routes.java @@ -18,10 +18,10 @@ import java.util.ArrayList; import java.util.Arrays; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; import spark.FilterImpl; import spark.RouteImpl; diff --git a/src/test/java/spark/route/RoutesConcurrencyTest.java b/src/test/java/spark/route/RoutesConcurrencyTest.java index 58ffafdf2c..36491d95b7 100644 --- a/src/test/java/spark/route/RoutesConcurrencyTest.java +++ b/src/test/java/spark/route/RoutesConcurrencyTest.java @@ -1,7 +1,6 @@ package spark.route; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; @@ -12,19 +11,24 @@ import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ErrorCollector; import spark.FilterImpl; import spark.RouteImpl; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; public class RoutesConcurrencyTest { private final AtomicInteger numberOfSuccessfulIterations = new AtomicInteger(); private final AtomicInteger numberOfFailedIterations = new AtomicInteger(); private ExecutorService executorService; + @Rule + public final ErrorCollector collector = new ErrorCollector(); + private static final int NUMBER_OF_ITERATIONS = 10_000; private static final int NUMBER_OF_THREADS = 2; private static final int NUMBER_OF_TASKS = NUMBER_OF_THREADS; @@ -55,19 +59,19 @@ public void teardown() { @Test public void classShouldBeThreadSafe() throws Exception { executorService = Executors.newFixedThreadPool(NUMBER_OF_THREADS); - Collection> tasks = partitionIterationsIntoTasks(); + List> tasks = partitionIterationsIntoTasks(); List> futureResults = executorService.invokeAll(tasks); executorService.shutdown(); for (Future futureResult : futureResults) { futureResult.get(); - assertTrue(futureResult.isDone()); + collector.checkThat(futureResult.isDone(), is(true)); } - assertEquals(NUMBER_OF_ITERATIONS, numberOfSuccessfulIterations.intValue()); - assertEquals(0, numberOfFailedIterations.intValue()); + collector.checkThat(NUMBER_OF_ITERATIONS, equalTo(numberOfSuccessfulIterations.intValue())); + collector.checkThat(0, equalTo(numberOfFailedIterations.intValue())); } - private Collection> partitionIterationsIntoTasks() { - final Collection> tasks = new ArrayList<>(); + private List> partitionIterationsIntoTasks() { + final List> tasks = new ArrayList<>(); final Routes routes = Routes.create(); final int numberOfIterationsPerTask = NUMBER_OF_ITERATIONS / NUMBER_OF_TASKS; for (int taskIndex = 0; taskIndex < NUMBER_OF_TASKS; taskIndex++) { From b5572807d674f87eece5eaf32ccd990bd24f47ce Mon Sep 17 00:00:00 2001 From: Mario Catalan Date: Tue, 9 Mar 2021 21:53:30 +0100 Subject: [PATCH 4/4] Fixed order of actual and expected values in assertions --- src/test/java/spark/route/RoutesConcurrencyTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/spark/route/RoutesConcurrencyTest.java b/src/test/java/spark/route/RoutesConcurrencyTest.java index 36491d95b7..2fd97086a2 100644 --- a/src/test/java/spark/route/RoutesConcurrencyTest.java +++ b/src/test/java/spark/route/RoutesConcurrencyTest.java @@ -66,8 +66,8 @@ public void classShouldBeThreadSafe() throws Exception { futureResult.get(); collector.checkThat(futureResult.isDone(), is(true)); } - collector.checkThat(NUMBER_OF_ITERATIONS, equalTo(numberOfSuccessfulIterations.intValue())); - collector.checkThat(0, equalTo(numberOfFailedIterations.intValue())); + collector.checkThat(numberOfSuccessfulIterations.intValue(), equalTo(NUMBER_OF_ITERATIONS)); + collector.checkThat(numberOfFailedIterations.intValue(), equalTo(0)); } private List> partitionIterationsIntoTasks() {