imbajin commented on code in PR #3167:
URL: https://github.com/apache/hugegraph/pull/3167#discussion_r3840564888
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/algorithm/ShortestPathTraverser.java:
##########
@@ -184,33 +185,36 @@ public PathSet forward(boolean all) {
while (this.pathResults.hasNextKey()) {
Id source = this.pathResults.nextKey();
- Iterator<Edge> edges = edgesOfVertex(source, this.direction,
- this.labels, degree);
- edges = skipSuperNodeIfNeeded(edges, this.degree,
- this.skipDegree);
+ Iterator<Edge> sourceEdges = edgesOfVertex(
Review Comment:
⚠️ Important
Blocking: yes. `edgesOfVertex(...)` is called before the `try`. For multiple
labels, `HugeTraverser` builds an `ExtendableIterator` and opens one backend
iterator per label; if a later label query throws, earlier closeable HStore
iterators are already owned by that local aggregator but are lost before this
`finally` is installed. The same acquisition pattern is used by `backward()`.
Please make multi-label iterator acquisition close all previously opened
children on failure (or provide an exception-safe acquisition API) before
relying on this cleanup block.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/traversal/ShortestPathTraverserTest.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.unit.traversal;
+
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.Iterator;
+import java.util.Map;
+
+import org.apache.hugegraph.HugeGraph;
+import org.apache.hugegraph.backend.id.EdgeId;
+import org.apache.hugegraph.backend.id.Id;
+import org.apache.hugegraph.backend.id.IdGenerator;
+import org.apache.hugegraph.config.CoreOptions;
+import org.apache.hugegraph.structure.HugeEdge;
+import org.apache.hugegraph.testutil.Assert;
+import org.apache.hugegraph.traversal.algorithm.HugeTraverser.Path;
+import org.apache.hugegraph.traversal.algorithm.ShortestPathTraverser;
+import org.apache.hugegraph.type.define.CollectionType;
+import org.apache.hugegraph.type.define.Directions;
+import org.apache.hugegraph.unit.BaseUnitTest;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class ShortestPathTraverserTest extends BaseUnitTest {
+
+ @Test
+ public void testCloseEdgesWhenPathFoundForward() {
+ Id source = IdGenerator.of(1L);
+ Id target = IdGenerator.of(2L);
+ TrackingIterator edges = edges(edgeTo(target));
+ TestTraverser traverser = new TestTraverser(edges);
+
+ Path path = shortestPath(traverser, source, target, 1, 0L);
+
+ Assert.assertEquals(Arrays.asList(source, target), path.vertices());
+ Assert.assertTrue(edges.closed());
+ }
+
+ @Test
+ public void testCloseEdgesWhenPathFoundBackward() {
+ Id source = IdGenerator.of(1L);
+ Id middle = IdGenerator.of(2L);
+ Id target = IdGenerator.of(3L);
+ TrackingIterator forwardEdges = edges(edgeTo(middle));
+ TrackingIterator backwardEdges = edges(edgeTo(middle));
+ TestTraverser traverser = new TestTraverser(forwardEdges,
+ backwardEdges);
+
+ Path path = shortestPath(traverser, source, target, 2, 0L);
+
+ Assert.assertEquals(Arrays.asList(source, middle, target),
+ path.vertices());
+ Assert.assertTrue(forwardEdges.closed());
+ Assert.assertTrue(backwardEdges.closed());
+ }
+
+ @Test
+ public void testCloseEdgesWhenCheckingSuperNode() {
+ Id source = IdGenerator.of(1L);
+ Id target = IdGenerator.of(2L);
+ TrackingIterator sourceEdges = edges(edgeTo(target));
Review Comment:
⚠️ Important
Blocking: no. This test supplies one source edge while `skipDegree=2`, so
`skipSuperNodeIfNeeded()` never reaches its threshold branch that returns the
empty iterator; it also uses a raw `TrackingIterator`, empty labels, and only
`shortestPath(..., all=false)`. The added tests therefore do not prove cleanup
for the actual super-node skip, wrapper chain, `allShortestPaths()`, or
iterator exception paths. Please add threshold, wrapped-backend,
full-traversal, and throwing-iterator cases with close assertions.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/algorithm/ShortestPathTraverser.java:
##########
@@ -184,33 +185,36 @@ public PathSet forward(boolean all) {
while (this.pathResults.hasNextKey()) {
Id source = this.pathResults.nextKey();
- Iterator<Edge> edges = edgesOfVertex(source, this.direction,
- this.labels, degree);
- edges = skipSuperNodeIfNeeded(edges, this.degree,
- this.skipDegree);
+ Iterator<Edge> sourceEdges = edgesOfVertex(
+ source, this.direction, this.labels, degree);
+ try {
+ Iterator<Edge> edges = skipSuperNodeIfNeeded(
+ sourceEdges, this.degree, this.skipDegree);
- this.vertexCount += 1L;
+ this.vertexCount += 1L;
- while (edges.hasNext()) {
- HugeEdge edge = (HugeEdge) edges.next();
- Id target = edge.id().otherVertexId();
+ while (edges.hasNext()) {
+ HugeEdge edge = (HugeEdge) edges.next();
+ Id target = edge.id().otherVertexId();
- this.edgeResults.addEdge(source, target, edge);
+ this.edgeResults.addEdge(source, target, edge);
- PathSet paths = this.pathResults.findPath(target,
- t ->
!this.superNode(t,
-
this.direction),
- all, false);
+ PathSet paths = this.pathResults.findPath(
+ target,
+ t -> !this.superNode(t, this.direction),
+ all, false);
- if (paths.isEmpty()) {
- continue;
- }
- results.addAll(paths);
- if (!all) {
- return paths;
+ if (paths.isEmpty()) {
+ continue;
+ }
+ results.addAll(paths);
+ if (!all) {
+ return paths;
+ }
}
+ } finally {
Review Comment:
⚠️ Important
Blocking: no. `CloseableIterator.closeIterator()` wraps an
`AutoCloseable.close()` failure in a `RuntimeException`. In these `finally`
blocks, that cleanup exception replaces an exception already thrown by
`hasNext()`, `next()`, or path matching, so a backend query failure can be
reported as a close failure; the same applies to the backward and super-node
cleanup blocks. Please preserve the primary exception and attach a close
failure as suppressed, propagating it directly only when no primary exception
exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
