Merge pull request #388 from neo4j-contrib/0.28-neo4j-4.3

Port Spatial 0.28 to Neo4j 4.3

README.md

@@ -81,6 +81,9 @@ This has meant that the spatial library needed a major refactoring to work with
   0.28.0 solves this by using different indexes, and keeping all imports completely separate.
   The more complex problems of importing newer versions, and stitching together overlapping areas, are not
   yet solved.
+* Neo4j 4.3 has an issue with leaking RelationshipTraversalCursor, and we needed to do some workarounds to
+  avoid this issue, usually by exhausting the iterator, which can have a higher performance cost in some cases.
+  Version 0.28.1 includes this fix.
 
 Consequences of the port to Neo4j 4.x:
 
@@ -355,6 +358,7 @@ The Neo4j Spatial Plugin is available for inclusion in the server version of Neo
   * [v0.27.1 for Neo4j 4.1.7](https://github.com/neo4j-contrib/m2/blob/master/releases/org/neo4j/neo4j-spatial/0.27.1-neo4j-4.1.7/neo4j-spatial-0.27.1-neo4j-4.1.7-server-plugin.jar?raw=true)
   * [v0.27.2 for Neo4j 4.2.3](https://github.com/neo4j-contrib/m2/blob/master/releases/org/neo4j/neo4j-spatial/0.27.2-neo4j-4.2.3/neo4j-spatial-0.27.2-neo4j-4.2.3-server-plugin.jar?raw=true)
   * [v0.28.0 for Neo4j 4.2.3](https://github.com/neo4j-contrib/m2/blob/master/releases/org/neo4j/neo4j-spatial/0.28.0-neo4j-4.2.3/neo4j-spatial-0.28.0-neo4j-4.2.3-server-plugin.jar?raw=true)
+  * [v0.28.1 for Neo4j 4.3.10](https://github.com/neo4j-contrib/m2/blob/master/releases/org/neo4j/neo4j-spatial/0.28.1-neo4j-4.3.10/neo4j-spatial-0.28.1-neo4j-4.3.10-server-plugin.jar?raw=true)
 
 For versions up to 0.15-neo4j-2.3.4:
 
@@ -471,7 +475,7 @@ Add the following repositories and dependency to your project's pom.xml:
     <dependency>
         <groupId>org.neo4j</groupId>
         <artifactId>neo4j-spatial</artifactId>
-        <version>0.28.0-neo4j-4.2.3</version>
+        <version>0.28.1-neo4j-4.3.10</version>
     </dependency>
 ~~~
 

pom.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
     <properties>
-        <neo4j.version>4.2.3</neo4j.version>
+        <neo4j.version>4.3.10</neo4j.version>
         <lucene.version>8.2.0</lucene.version>
         <!-- make sure lucene version is the same as the one the current neo4j depends on -->
         <neo4j.java.version>11</neo4j.java.version>
@@ -23,7 +23,7 @@
     <modelVersion>4.0.0</modelVersion>
     <artifactId>neo4j-spatial</artifactId>
     <groupId>org.neo4j</groupId>
-    <version>0.28.0-neo4j-4.2.3</version>
+    <version>0.28.1-neo4j-4.3.10</version>
     <name>Neo4j - Spatial Components</name>
     <description>Spatial utilities and components for Neo4j</description>
     <url>http://components.neo4j.org/${project.artifactId}/${project.version}</url>

src/main/java/org/neo4j/gis/spatial/index/LayerSpaceFillingCurvePointIndex.java

@@ -35,10 +35,10 @@
 import org.neo4j.internal.schema.IndexDescriptor;
 import org.neo4j.internal.schema.IndexType;
 import org.neo4j.internal.schema.SchemaDescriptor;
-import org.neo4j.io.pagecache.tracing.cursor.PageCursorTracer;
+import org.neo4j.io.pagecache.context.CursorContext;
 import org.neo4j.kernel.api.KernelTransaction;
 import org.neo4j.kernel.impl.core.NodeEntity;
-import org.neo4j.kernel.impl.coreapi.internal.NodeCursorResourceIterator;
+import org.neo4j.kernel.impl.coreapi.internal.CursorIterator;
 import org.neo4j.memory.EmptyMemoryTracker;
 import org.opengis.referencing.crs.CoordinateReferenceSystem;
 import org.opengis.referencing.cs.CoordinateSystemAxis;
@@ -48,6 +48,7 @@
 import java.util.List;
 
 import static org.neo4j.internal.helpers.collection.Iterators.emptyResourceIterator;
+import static org.neo4j.kernel.api.ResourceTracker.EMPTY_RESOURCE_TRACKER;
 
 public abstract class LayerSpaceFillingCurvePointIndex extends ExplicitIndexBackedPointIndex<Long> {
 
@@ -120,13 +121,13 @@ public Iterator<Node> search(KernelTransaction ktx, Label label, String property
             int propId = ktx.tokenRead().propertyKey(propertyKey);
             ArrayList<Iterator<Node>> results = new ArrayList<>();
             for(SpaceFillingCurve.LongRange range:tiles) {
-                IndexQuery indexQuery = IndexQuery.range(propId, range.min, true, range.max, true);
+                PropertyIndexQuery indexQuery = PropertyIndexQuery.range(propId, range.min, true, range.max, true);
                 results.add(nodesByLabelAndProperty(ktx, labelId, indexQuery));
             }
             return Iterators.concat(results.iterator());
         }
 
-        private ResourceIterator<Node> nodesByLabelAndProperty(KernelTransaction transaction, int labelId, IndexQuery query )
+        private ResourceIterator<Node> nodesByLabelAndProperty(KernelTransaction transaction, int labelId, PropertyIndexQuery query )
         {
             Read read = transaction.dataRead();
 
@@ -146,11 +147,11 @@ private ResourceIterator<Node> nodesByLabelAndProperty(KernelTransaction transac
                 // Ha! We found an index - let's use it to find matching nodes
                 try
                 {
-                    NodeValueIndexCursor cursor = transaction.cursors().allocateNodeValueIndexCursor(PageCursorTracer.NULL, EmptyMemoryTracker.INSTANCE);
+                    NodeValueIndexCursor cursor = transaction.cursors().allocateNodeValueIndexCursor(CursorContext.NULL, EmptyMemoryTracker.INSTANCE);
                     IndexReadSession indexSession = read.indexReadSession( index );
                     read.nodeIndexSeek( indexSession, cursor, IndexQueryConstraints.unordered(false), query );
 
-                    return new NodeCursorResourceIterator<>( cursor, (id) -> new NodeEntity(transaction.internalTransaction(), id) );
+                    return new CursorIterator<>(cursor, NodeIndexCursor::nodeReference, (c) -> new NodeEntity(transaction.internalTransaction(), c.nodeReference()), EMPTY_RESOURCE_TRACKER);
                 }
                 catch ( KernelException e )
                 {

src/main/java/org/neo4j/gis/spatial/osm/OSMDataset.java

@@ -23,6 +23,7 @@
 import org.locationtech.jts.geom.Envelope;
 import org.locationtech.jts.geom.Geometry;
 import org.neo4j.gis.spatial.*;
+import org.neo4j.gis.spatial.utilities.RelationshipTraversal;
 import org.neo4j.graphdb.Direction;
 import org.neo4j.graphdb.Node;
 import org.neo4j.graphdb.Relationship;
@@ -144,8 +145,7 @@ public Node getUser(Node nodeWayOrChangeset) {
                 .relationships(OSMRelation.CHANGESET, Direction.OUTGOING)
                 .relationships(OSMRelation.USER, Direction.OUTGOING)
                 .evaluator(Evaluators.includeWhereLastRelationshipTypeIs(OSMRelation.USER));
-        Iterator<Node> results = td.traverse(nodeWayOrChangeset).nodes().iterator();
-        return results.hasNext() ? results.next() : null;
+        return RelationshipTraversal.getFirstNode(td.traverse(nodeWayOrChangeset).nodes());
     }
 
     public Way getWayFromId(Transaction tx, long id) {
@@ -161,8 +161,8 @@ public Way getWayFrom(Node osmNodeOrWayNodeOrGeomNode) {
                 .relationships(OSMRelation.GEOM, Direction.INCOMING)
                 .evaluator(path -> path.endNode().hasProperty("way_osm_id") ? Evaluation.INCLUDE_AND_PRUNE
                         : Evaluation.EXCLUDE_AND_CONTINUE);
-        Iterator<Node> results = td.traverse(osmNodeOrWayNodeOrGeomNode).nodes().iterator();
-        return results.hasNext() ? new Way(results.next()) : null;
+        Node node = RelationshipTraversal.getFirstNode(td.traverse(osmNodeOrWayNodeOrGeomNode).nodes());
+        return node != null ? new Way(node) : null;
     }
 
     public class OSMNode {

src/main/java/org/neo4j/gis/spatial/pipes/impl/AbstractPipe.java

@@ -45,6 +45,9 @@ public void reset() {
         if (this.starts instanceof Pipe) {
             ((Pipe) this.starts).reset();
         }
+        if (this.starts instanceof LastElementIterator) {
+            ((LastElementIterator) this.starts).reset();
+        }
         this.nextEnd = null;
         this.currentEnd = null;
         this.available = false;

src/main/java/org/neo4j/gis/spatial/pipes/impl/LastElementIterator.java

@@ -1,5 +1,7 @@
 package org.neo4j.gis.spatial.pipes.impl;
 
+import org.neo4j.gis.spatial.utilities.RelationshipTraversal;
+
 import java.util.Iterator;
 
 public class LastElementIterator<T> implements Iterator<T> {
@@ -26,4 +28,14 @@ public void remove() {
     public T lastElement() {
         return lastElement;
     }
+
+    /**
+     * Work around bug in Neo4j 4.3 with leaked RelationshipTraversalCursor
+     */
+    public void reset() {
+        // TODO: rather try get deeper into the underlying index and close resources instead of exhausting the iterator
+        // The challenge is that there are many sources, all of which need to be made closable, and that is very hard
+        // to achieve in a generic way without a full-stack code change.
+        RelationshipTraversal.exhaustIterator(source);
+    }
 }

src/main/java/org/neo4j/gis/spatial/pipes/impl/Pipeline.java

@@ -127,6 +127,7 @@ public int size() {
     }
 
     public void reset() {
+        this.startPipe.reset(); // Clear incoming state to avoid bug in Neo4j 4.3 with leaked RelationshipTraversalCursor
         this.endPipe.reset();
     }
 

src/main/java/org/neo4j/gis/spatial/utilities/RelationshipTraversal.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2010-2022 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j Spatial.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.gis.spatial.utilities;
+
+import org.neo4j.graphdb.*;
+
+import java.util.Iterator;
+
+/**
+ * Neo4j 4.3 introduced some bugs around closing RelationshipTraversalCursor.
+ * This class provides alternative implementations of suspicious methods to work around that issue.
+ */
+public class RelationshipTraversal {
+    /**
+     * Normally just calling iterator.next() once should work, but the bug in Neo4j 4.3 results in leaked cursors
+     * if this iterator comes from the traversal framework, so this code exhausts the traverser and returns the first
+     * node found. For cases where many results could be found, this is expensive. Try to use only when one or few results
+     * are likely.
+     */
+    public static Node getFirstNode(Iterable<Node> nodes) {
+        Node found = null;
+        for (Node node : nodes) {
+            if (found == null) found = node;
+        }
+        return found;
+    }
+
+    /**
+     * Some code has facilities for closing resource at a high level, but the underlying resources are only
+     * Iterators, with no access to the original sources and no way to close the resources properly.
+     * So to avoid the Neo4j 4.3 bug with leaked RelationshipTraversalCursor, we need to exhaust the iterator.
+     */
+    public static void exhaustIterator(Iterator source) {
+        while (source.hasNext()) {
+            source.next();
+        }
+    }
+}

src/test/java/org/neo4j/gis/spatial/Neo4jTestCase.java

@@ -23,16 +23,17 @@
 import org.junit.Before;
 import org.junit.Rule;
 import org.neo4j.configuration.Config;
+import org.neo4j.configuration.GraphDatabaseInternalSettings;
 import org.neo4j.configuration.GraphDatabaseSettings;
 import org.neo4j.dbms.api.DatabaseManagementService;
-import org.neo4j.dbms.api.DatabaseManagementServiceBuilder;
 import org.neo4j.gis.spatial.procedures.SpatialProcedures;
 import org.neo4j.graphdb.GraphDatabaseService;
 import org.neo4j.io.fs.FileUtils;
 import org.neo4j.io.layout.DatabaseLayout;
 import org.neo4j.io.layout.Neo4jLayout;
 import org.neo4j.kernel.api.procedure.GlobalProcedures;
 import org.neo4j.kernel.internal.GraphDatabaseAPI;
+import org.neo4j.test.TestDatabaseManagementServiceBuilder;
 import org.neo4j.test.rule.fs.EphemeralFileSystemRule;
 
 import java.io.File;
@@ -57,6 +58,7 @@ public abstract class Neo4jTestCase {
         //NORMAL_CONFIG.put( GraphDatabaseSettings.strings_mapped_memory_size.name(), "200M" );
         //NORMAL_CONFIG.put( GraphDatabaseSettings.arrays_mapped_memory_size.name(), "0M" );
         NORMAL_CONFIG.put(GraphDatabaseSettings.pagecache_memory.name(), "200M");
+        NORMAL_CONFIG.put(GraphDatabaseInternalSettings.trace_cursors.name(), "true");
     }
 
     static final Map<String, String> LARGE_CONFIG = new HashMap<>();
@@ -100,7 +102,7 @@ protected void setUp(boolean deleteDb) throws Exception {
         if (largeMode != null && largeMode.equalsIgnoreCase("true")) {
             config = LARGE_CONFIG;
         }
-        databases = new DatabaseManagementServiceBuilder(getDbPath()).setConfigRaw(config).build();
+        databases = new TestDatabaseManagementServiceBuilder(getDbPath()).setConfigRaw(config).build();
         graphDb = databases.database(DEFAULT_DATABASE_NAME);
         ((GraphDatabaseAPI) graphDb).getDependencyResolver().resolveDependency(GlobalProcedures.class).registerProcedure(SpatialProcedures.class);
     }

src/test/java/org/neo4j/gis/spatial/OsmAnalysisTest.java

@@ -112,40 +112,8 @@ public void runTest() throws Exception {
         runAnalysis(layerName, years, days);
     }
 
-    private DatabaseManagementService databases;
-    private GraphDatabaseService db;
-
-    protected GraphDatabaseService graphDb() {
-        return db == null ? super.graphDb() : db;
-    }
-
-    protected void shutdownDatabase() {
-        if (db != null) {
-            databases.shutdown();
-            databases = null;
-            db = null;
-        }
-    }
-
-    protected SpatialDatabaseService setDataset(String dataset) {
-        if (db != null) {
-            shutdownDatabase();
-        }
-        File dbDir = new File("var", dataset);
-        if (dbDir.exists()) {
-            try {
-                FileUtils.deleteDirectory(dbDir);
-            } catch (IOException e) {
-                System.out.println("Failed to delete previous database directory '" + dbDir + "': " + e.getMessage());
-            }
-        }
-        databases = new TestDatabaseManagementServiceBuilder(dbDir.toPath()).impermanent().build();
-        db = databases.database(DEFAULT_DATABASE_NAME);
-        return new SpatialDatabaseService(new IndexManager((GraphDatabaseAPI) db, SecurityContext.AUTH_DISABLED));
-    }
-
     protected void runAnalysis(String osm, int years, int days) throws Exception {
-        SpatialDatabaseService spatial = setDataset(osm);
+        SpatialDatabaseService spatial = new SpatialDatabaseService(new IndexManager((GraphDatabaseAPI) graphDb(), SecurityContext.AUTH_DISABLED));
         boolean alreadyImported;
         try (Transaction tx = graphDb().beginTx()) {
             alreadyImported = spatial.getLayer(tx, osm) != null;
@@ -155,11 +123,10 @@ protected void runAnalysis(String osm, int years, int days) throws Exception {
             runImport(osm, usePoints);
         }
         testAnalysis2(osm, years, days);
-        shutdownDatabase();
     }
 
     public void testAnalysis2(String osm, int years, int days) throws IOException {
-        SpatialDatabaseService spatial = new SpatialDatabaseService(new IndexManager((GraphDatabaseAPI) db, SecurityContext.AUTH_DISABLED));
+        SpatialDatabaseService spatial = new SpatialDatabaseService(new IndexManager((GraphDatabaseAPI) graphDb(), SecurityContext.AUTH_DISABLED));
         LinkedHashMap<DynamicLayerConfig, Long> slides = new LinkedHashMap<>();
         Map<String, User> userIndex = new HashMap<>();
         int user_rank = 1;
@@ -279,7 +246,7 @@ public void testAnalysis(String osm) throws Exception {
             Map<String, User> userIndex = collectUserChangesetData(usersNode);
             SortedSet<User> topTen = getTopTen(userIndex);
 
-            SpatialDatabaseService spatial = new SpatialDatabaseService(new IndexManager((GraphDatabaseAPI) db, SecurityContext.AUTH_DISABLED));
+            SpatialDatabaseService spatial = new SpatialDatabaseService(new IndexManager((GraphDatabaseAPI) graphDb(), SecurityContext.AUTH_DISABLED));
             layers = exportPoints(tx, osm, spatial, topTen);
 
             layers = removeEmptyLayers(tx, layers);

src/test/java/org/neo4j/gis/spatial/RTreeBulkInsertTest.java

@@ -1,12 +1,13 @@
 package org.neo4j.gis.spatial;
 
-import org.locationtech.jts.geom.Coordinate;
-import org.locationtech.jts.geom.Geometry;
 import org.apache.commons.io.FileUtils;
 import org.geotools.data.neo4j.Neo4jFeatureBuilder;
 import org.geotools.referencing.crs.DefaultEngineeringCRS;
 import org.junit.*;
+import org.locationtech.jts.geom.Coordinate;
+import org.locationtech.jts.geom.Geometry;
 import org.neo4j.dbms.api.DatabaseManagementService;
+import org.neo4j.dbms.api.DatabaseManagementServiceBuilder;
 import org.neo4j.gis.spatial.encoders.SimplePointEncoder;
 import org.neo4j.gis.spatial.index.*;
 import org.neo4j.gis.spatial.pipes.GeoPipeFlow;
@@ -16,7 +17,6 @@
 import org.neo4j.graphdb.*;
 import org.neo4j.internal.kernel.api.security.SecurityContext;
 import org.neo4j.kernel.internal.GraphDatabaseAPI;
-import org.neo4j.test.TestDatabaseManagementServiceBuilder;
 import org.opengis.feature.simple.SimpleFeatureType;
 import org.opengis.referencing.FactoryException;
 import org.opengis.referencing.crs.CoordinateReferenceSystem;
@@ -1582,7 +1582,7 @@ private void restart() throws IOException {
             FileUtils.deleteDirectory(storeDir);
         }
         FileUtils.forceMkdir(storeDir);
-        databases = new TestDatabaseManagementServiceBuilder(storeDir.toPath()).impermanent().build();
+        databases = new DatabaseManagementServiceBuilder(storeDir.toPath()).build();
         db = databases.database(DEFAULT_DATABASE_NAME);
     }
 

src/test/java/org/neo4j/gis/spatial/TestOSMImport.java

@@ -134,9 +134,10 @@ public static void checkOSMSearch(Transaction tx, OSMLayer layer) {
         Way way = null;
         int count = 0;
         for (Way wayNode : osm.getWays(tx)) {
-            way = wayNode;
-            if (count++ > 100)
-                break;
+            // Do not `break` from the loop or experience the RelationshipTraversalCursor leak bug in Neo4j 4.3
+            if (count++ <= 100) {
+                way = wayNode;
+            }
         }
         assertNotNull("Should be at least one way", way);
         Envelope bbox = way.getEnvelope();