Repository: graphhopper/graphhopper
Pull request #918 (2016-12-22 14:22:17)

Test concurrent graph change api while routing
To test #845 better I introduced one new concurrent test. But this looks rather complicated, maybe I did something wrong.

michaz: This is really tricky. Hold on a bit, I'm onto this.
karussell: I made the test more stable with the barrier. Also removed the second thread and use an executor service for better exception handling.

Patch:
diff --git a/core/src/main/java/com/graphhopper/GraphHopper.java b/core/src/main/java/com/graphhopper/GraphHopper.java
index 36e5118ac..de9691362 100644
--- a/core/src/main/java/com/graphhopper/GraphHopper.java
+++ b/core/src/main/java/com/graphhopper/GraphHopper.java
@@ -1113,7 +1113,7 @@ public class GraphHopper implements GraphHopperAPI {
         Lock writeLock = readWriteLock.writeLock();
         writeLock.lock();
         try {
-            ChangeGraphHelper overlay = new ChangeGraphHelper(ghStorage, locationIndex);
+            ChangeGraphHelper overlay = createChangeGraphHelper(ghStorage, locationIndex);
             long updateCount = overlay.applyChanges(encodingManager, collection);
             return new ChangeGraphResponse(updateCount);
         } finally {
@@ -1121,6 +1121,10 @@ public class GraphHopper implements GraphHopperAPI {
         }
     }
 
+    protected ChangeGraphHelper createChangeGraphHelper(Graph graph, LocationIndex locationIndex) {
+        return new ChangeGraphHelper(graph, locationIndex);
+    }
+
     private void checkIfPointsAreInBounds(List<GHPoint> points) {
         BBox bounds = getGraphHopperStorage().getBounds();
         for (int i = 0; i < points.size(); i++) {


Test patch:
diff --git a/reader-osm/src/test/java/com/graphhopper/GraphHopperAPITest.java b/core/src/test/java/com/graphhopper/GraphHopperAPITest.java
similarity index 54%
rename from reader-osm/src/test/java/com/graphhopper/GraphHopperAPITest.java
rename to core/src/test/java/com/graphhopper/GraphHopperAPITest.java
index 6ae541b49..8d4ab3e51 100644
--- a/reader-osm/src/test/java/com/graphhopper/GraphHopperAPITest.java
+++ b/core/src/test/java/com/graphhopper/GraphHopperAPITest.java
@@ -17,14 +17,25 @@
  */
 package com.graphhopper;
 
-import com.graphhopper.reader.osm.GraphHopperOSM;
+import com.graphhopper.json.geo.JsonFeature;
 import com.graphhopper.routing.util.EncodingManager;
+import com.graphhopper.storage.Graph;
 import com.graphhopper.storage.GraphBuilder;
 import com.graphhopper.storage.GraphHopperStorage;
 import com.graphhopper.storage.NodeAccess;
+import com.graphhopper.storage.change.ChangeGraphHelper;
+import com.graphhopper.storage.index.LocationIndex;
 import com.graphhopper.util.PointList;
+import com.graphhopper.util.shapes.BBox;
 import org.junit.Test;
 
+import java.util.*;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
 import static org.junit.Assert.*;
 
 /**
@@ -33,23 +44,30 @@ import static org.junit.Assert.*;
 public class GraphHopperAPITest {
     final EncodingManager encodingManager = new EncodingManager("car");
 
-    @Test
-    public void testLoad() {
-        GraphHopperStorage graph = new GraphBuilder(encodingManager).create();
+    void initGraph(GraphHopperStorage graph) {
         NodeAccess na = graph.getNodeAccess();
         na.setNode(0, 42, 10);
         na.setNode(1, 42.1, 10.1);
         na.setNode(2, 42.1, 10.2);
         na.setNode(3, 42, 10.4);
-        na.setNode(4, 41.9, 10.2);
 
         graph.edge(0, 1, 10, true);
-        graph.edge(1, 2, 10, false);
         graph.edge(2, 3, 10, true);
+    }
+
+    @Test
+    public void testLoad() {
+        GraphHopperStorage graph = new GraphBuilder(encodingManager).create();
+        initGraph(graph);
+        // do further changes:
+        NodeAccess na = graph.getNodeAccess();
+        na.setNode(4, 41.9, 10.2);
+
+        graph.edge(1, 2, 10, false);
         graph.edge(0, 4, 40, true);
         graph.edge(4, 3, 40, true);
 
-        GraphHopper instance = new GraphHopperOSM().
+        GraphHopper instance = new GraphHopper().
                 setStoreOnFlush(false).
                 setEncodingManager(encodingManager).setCHEnabled(false).
                 loadGraph(graph);
@@ -70,14 +88,7 @@ public class GraphHopperAPITest {
     @Test
     public void testDisconnected179() {
         GraphHopperStorage graph = new GraphBuilder(encodingManager).create();
-        NodeAccess na = graph.getNodeAccess();
-        na.setNode(0, 42, 10);
-        na.setNode(1, 42.1, 10.1);
-        na.setNode(2, 42.1, 10.2);
-        na.setNode(3, 42, 10.4);
-
-        graph.edge(0, 1, 10, true);
-        graph.edge(2, 3, 10, true);
+        initGraph(graph);
 
         GraphHopper instance = new GraphHopper().
                 setStoreOnFlush(false).
@@ -115,4 +126,66 @@ public class GraphHopperAPITest {
             assertTrue(ex.getMessage(), ex.getMessage().startsWith("Do a successful call to load or importOrLoad before routing"));
         }
     }
+
+    @Test
+    public void testConcurrentGraphChange() throws InterruptedException {
+        final GraphHopperStorage graph = new GraphBuilder(encodingManager).create();
+        initGraph(graph);
+        graph.edge(1, 2, 10, true);
+
+        final CountDownLatch latch = new CountDownLatch(1);
+        final AtomicInteger checkPointCounter = new AtomicInteger(0);
+        final GraphHopper graphHopper = new GraphHopper() {
+            @Override
+            protected ChangeGraphHelper createChangeGraphHelper(Graph graph, LocationIndex locationIndex) {
+                return new ChangeGraphHelper(graph, locationIndex) {
+                    @Override
+                    public long applyChanges(EncodingManager em, Collection<JsonFeature> features) {
+                        // force sleep inside the lock and let the main thread run until the lock barrier
+                        latch.countDown();
+                        try {
+                            Thread.sleep(400);
+                        } catch (InterruptedException e) {
+                            throw new RuntimeException(e);
+                        }
+                        checkPointCounter.incrementAndGet();
+                        return super.applyChanges(em, features);
+                    }
+                };
+            }
+        }.setStoreOnFlush(false).setEncodingManager(encodingManager).setCHEnabled(false).
+                loadGraph(graph);
+
+        GHResponse rsp = graphHopper.route(new GHRequest(42, 10.4, 42, 10));
+        assertFalse(rsp.toString(), rsp.hasErrors());
+        assertEquals(1800, rsp.getBest().getTime());
+
+        final List<JsonFeature> list = new ArrayList<>();
+        Map<String, Object> properties = new HashMap<>();
+        properties.put("speed", 5);
+
+        list.add(new JsonFeature("1", "bbox",
+                new BBox(10.399, 10.4, 42.0, 42.001),
+                null, properties));
+
+        ExecutorService executorService = Executors.newFixedThreadPool(1);
+        executorService.submit(new Runnable() {
+            @Override
+            public void run() {
+                graphHopper.changeGraph(list);
+                checkPointCounter.incrementAndGet();
+            }
+        });
+
+        latch.await();
+        assertEquals(0, checkPointCounter.get());
+        rsp = graphHopper.route(new GHRequest(42, 10.4, 42, 10));
+        assertFalse(rsp.toString(), rsp.hasErrors());
+        assertEquals(8400, rsp.getBest().getTime());
+
+        executorService.shutdown();
+        executorService.awaitTermination(3, TimeUnit.SECONDS);
+
+        assertEquals(2, checkPointCounter.get());
+    }
 }
diff --git a/reader-osm/src/test/java/com/graphhopper/reader/osm/OSMReaderTest.java b/reader-osm/src/test/java/com/graphhopper/reader/osm/OSMReaderTest.java
index bc5b18bf6..58408b3ed 100644
--- a/reader-osm/src/test/java/com/graphhopper/reader/osm/OSMReaderTest.java
+++ b/reader-osm/src/test/java/com/graphhopper/reader/osm/OSMReaderTest.java
@@ -92,7 +92,7 @@ public class OSMReaderTest {
 
     @Test
     public void testMain() {
-        GraphHopper hopper = new GraphHopperTest(file1).importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade(file1).importOrLoad();
         GraphHopperStorage graph = hopper.getGraphHopperStorage();
 
         assertNotNull(graph.getProperties().get("datareader.import.date"));
@@ -153,7 +153,7 @@ public class OSMReaderTest {
 
     @Test
     public void testSort() {
-        GraphHopper hopper = new GraphHopperTest(file1).setSortGraph(true).importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade(file1).setSortGraph(true).importOrLoad();
         NodeAccess na = hopper.getGraphHopperStorage().getNodeAccess();
         assertEquals(10, na.getLongitude(findID(hopper.getLocationIndex(), 49, 10)), 1e-3);
         assertEquals(51.249, na.getLatitude(findID(hopper.getLocationIndex(), 51.2492152, 9.4317166)), 1e-3);
@@ -161,7 +161,7 @@ public class OSMReaderTest {
 
     @Test
     public void testWithBounds() {
-        GraphHopper hopper = new GraphHopperTest(file1) {
+        GraphHopper hopper = new GraphHopperFacade(file1) {
             @Override
             protected DataReader createReader(GraphHopperStorage tmpGraph) {
                 return new OSMReader(tmpGraph) {
@@ -208,7 +208,7 @@ public class OSMReaderTest {
 
     @Test
     public void testOneWay() {
-        GraphHopper hopper = new GraphHopperTest(file2).importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade(file2).importOrLoad();
         GraphHopperStorage graph = hopper.getGraphHopperStorage();
 
         assertEquals("2014-01-02T01:10:14Z", graph.getProperties().get("datareader.data.date"));
@@ -257,7 +257,7 @@ public class OSMReaderTest {
 
     @Test
     public void testFerry() {
-        GraphHopper hopper = new GraphHopperTest(file2) {
+        GraphHopper hopper = new GraphHopperFacade(file2) {
             @Override
             public void cleanUp() {
             }
@@ -283,7 +283,7 @@ public class OSMReaderTest {
 
     @Test
     public void testMaxSpeed() {
-        GraphHopper hopper = new GraphHopperTest(file2) {
+        GraphHopper hopper = new GraphHopperFacade(file2) {
             @Override
             public void cleanUp() {
             }
@@ -298,7 +298,7 @@ public class OSMReaderTest {
 
     @Test
     public void testWayReferencesNotExistingAdjNode() {
-        GraphHopper hopper = new GraphHopperTest(file4).importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade(file4).importOrLoad();
         Graph graph = hopper.getGraphHopperStorage();
 
         assertEquals(2, graph.getNodes());
@@ -310,7 +310,7 @@ public class OSMReaderTest {
 
     @Test
     public void testFoot() {
-        GraphHopper hopper = new GraphHopperTest(file3).importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade(file3).importOrLoad();
         Graph graph = hopper.getGraphHopperStorage();
 
         int n10 = AbstractGraphStorageTester.getIdOf(graph, 11.1);
@@ -332,7 +332,7 @@ public class OSMReaderTest {
 
     @Test
     public void testNegativeIds() {
-        GraphHopper hopper = new GraphHopperTest(fileNegIds).importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade(fileNegIds).importOrLoad();
         Graph graph = hopper.getGraphHopperStorage();
         assertEquals(4, graph.getNodes());
         int n20 = AbstractGraphStorageTester.getIdOf(graph, 52);
@@ -356,7 +356,7 @@ public class OSMReaderTest {
 
     @Test
     public void testBarriers() {
-        GraphHopper hopper = new GraphHopperTest(fileBarriers).
+        GraphHopper hopper = new GraphHopperFacade(fileBarriers).
                 setMinNetworkSize(0, 0).
                 importOrLoad();
 
@@ -393,7 +393,7 @@ public class OSMReaderTest {
 
     @Test
     public void testBarriersOnTowerNodes() {
-        GraphHopper hopper = new GraphHopperTest(fileBarriers).
+        GraphHopper hopper = new GraphHopperFacade(fileBarriers).
                 setMinNetworkSize(0, 0).
                 importOrLoad();
         Graph graph = hopper.getGraphHopperStorage();
@@ -446,7 +446,7 @@ public class OSMReaderTest {
 
     @Test
     public void testTurnRestrictions() {
-        GraphHopper hopper = new GraphHopperTest(fileTurnRestrictions, true).
+        GraphHopper hopper = new GraphHopperFacade(fileTurnRestrictions, true).
                 importOrLoad();
 
         Graph graph = hopper.getGraphHopperStorage();
@@ -580,7 +580,7 @@ public class OSMReaderTest {
 
     @Test
     public void testReadEleFromCustomOSM() {
-        GraphHopper hopper = new GraphHopperTest("custom-osm-ele.xml") {
+        GraphHopper hopper = new GraphHopperFacade("custom-osm-ele.xml") {
             @Override
             protected DataReader createReader(GraphHopperStorage tmpGraph) {
                 return initDataReader(new OSMReader(tmpGraph) {
@@ -602,7 +602,7 @@ public class OSMReaderTest {
 
     @Test
     public void testReadEleFromDataProvider() {
-        GraphHopper hopper = new GraphHopperTest("test-osm5.xml");
+        GraphHopper hopper = new GraphHopperFacade("test-osm5.xml");
         // get N10E046.hgt.zip
         ElevationProvider provider = new SRTMProvider();
         provider.setCacheDir(new File(GraphHopperIT.DIR));
@@ -705,14 +705,14 @@ public class OSMReaderTest {
 
     @Test
     public void testPreferredLanguage() {
-        GraphHopper hopper = new GraphHopperTest(file1).setPreferredLanguage("de").importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade(file1).setPreferredLanguage("de").importOrLoad();
         GraphHopperStorage graph = hopper.getGraphHopperStorage();
         int n20 = AbstractGraphStorageTester.getIdOf(graph, 52);
         EdgeIterator iter = carOutExplorer.setBaseNode(n20);
         assertTrue(iter.next());
         assertEquals("straße 123, B 122", iter.getName());
 
-        hopper = new GraphHopperTest(file1).setPreferredLanguage("el").importOrLoad();
+        hopper = new GraphHopperFacade(file1).setPreferredLanguage("el").importOrLoad();
         graph = hopper.getGraphHopperStorage();
         n20 = AbstractGraphStorageTester.getIdOf(graph, 52);
         iter = carOutExplorer.setBaseNode(n20);
@@ -723,7 +723,7 @@ public class OSMReaderTest {
 
     @Test
     public void testDataDateWithinPBF() {
-        GraphHopper hopper = new GraphHopperTest("test-osm6.pbf").importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade("test-osm6.pbf").importOrLoad();
         GraphHopperStorage graph = hopper.getGraphHopperStorage();
 
         assertEquals("2014-01-02T00:10:14Z", graph.getProperties().get("datareader.data.date"));
@@ -731,7 +731,7 @@ public class OSMReaderTest {
 
     @Test
     public void testCrossBoundary_issue667() {
-        GraphHopper hopper = new GraphHopperTest("test-osm-waterway.xml").importOrLoad();
+        GraphHopper hopper = new GraphHopperFacade("test-osm-waterway.xml").importOrLoad();
         QueryResult qr = hopper.getLocationIndex().findClosest(0.1, 179.5, EdgeFilter.ALL_EDGES);
         assertTrue(qr.isValid());
         assertEquals(0.1, qr.getSnappedPoint().lat, 0.1);
@@ -761,12 +761,12 @@ public class OSMReaderTest {
         assertFalse(ghRsp.getErrors().toString(), ghRsp.hasErrors());
     }
 
-    class GraphHopperTest extends GraphHopperOSM {
-        public GraphHopperTest(String osmFile) {
+    class GraphHopperFacade extends GraphHopperOSM {
+        public GraphHopperFacade(String osmFile) {
             this(osmFile, false);
         }
 
-        public GraphHopperTest(String osmFile, boolean turnCosts) {
+        public GraphHopperFacade(String osmFile, boolean turnCosts) {
             setStoreOnFlush(false);
             setOSMFile(osmFile);
             setGraphHopperLocation(dir);