This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-2626 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit dcca73d02bd16933d035f112204afac7652ea5c4 Author: Stephen Mallette <[email protected]> AuthorDate: Thu Dec 30 07:50:04 2021 -0500 TINKERPOP-2626 Refactored Traversal to include isClosed() The isClosed() method was added in a non-breaking way in 3.4.13 to fix a bug where traversal resources were not being released properly. Refactored here for 3.6.0 where isClosed() could be added to Traversal more consistently. CTR --- CHANGELOG.asciidoc | 1 + .../remote/traversal/EmbeddedRemoteTraversal.java | 5 +++++ .../gremlin/process/traversal/Traversal.java | 5 +++++ .../traversal/lambda/AbstractLambdaTraversal.java | 21 ++++++++++++++++++++- .../process/traversal/util/DefaultTraversal.java | 4 +--- .../process/traversal/util/EmptyTraversal.java | 8 ++++++++ .../gremlin/process/traversal/TraversalTest.java | 3 ++- .../driver/remote/DriverRemoteTraversal.java | 19 +++++++++++++++++++ 8 files changed, 61 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index fa45d0b..a8b46c1 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -43,6 +43,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Packaged Gherkin tests and data as standalone package as a convenience distribution. * Removed `ProductiveByStrategy` as a strategy that is applied by default. * Changed `by()` modulator semantics to consistently filter. +* Refactored `Traversal` interface to include `isClosed()` method. * Removed previously deprecated `application/vnd.gremlin-v1.0+gryo-lite` serialization format. * Removed previously deprecated `AuthenticationSettings.enableAuditLog`. * Removed previously deprecated `GroovyTranslator` from `gremlin-groovy` module. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/EmbeddedRemoteTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/EmbeddedRemoteTraversal.java index 9dc37e7..f16d046 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/EmbeddedRemoteTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/EmbeddedRemoteTraversal.java @@ -48,4 +48,9 @@ public class EmbeddedRemoteTraversal<S,E> extends AbstractRemoteTraversal<S,E> { public E next() { return t.next(); } + + @Override + public boolean isClosed() { + return t.isClosed(); + } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java index da221d5..de3c25c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java @@ -308,6 +308,11 @@ public interface Traversal<S, E> extends Iterator<E>, Serializable, Cloneable, A } /** + * Determines if the traversal has been fully iterated and resources released. + */ + public boolean isClosed(); + + /** * A collection of {@link Exception} types associated with Traversal execution. */ public static class Exceptions { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java index ac8919f..76513a1 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java @@ -31,10 +31,12 @@ import org.apache.tinkerpop.gremlin.process.traversal.traverser.B_O_TraverserGen import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.process.traversal.util.EmptyTraversalSideEffects; import org.apache.tinkerpop.gremlin.process.traversal.util.EmptyTraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException; import org.apache.tinkerpop.gremlin.structure.Graph; import java.util.Collections; import java.util.List; +import java.util.NoSuchElementException; import java.util.Optional; import java.util.Set; @@ -46,6 +48,7 @@ public abstract class AbstractLambdaTraversal<S, E> implements Traversal.Admin<S private static final Set<TraverserRequirement> REQUIREMENTS = Collections.singleton(TraverserRequirement.OBJECT); protected Traversal.Admin<S, E> bypassTraversal = null; + protected boolean closed = false; public void setBypassTraversal(final Traversal.Admin<S, E> bypassTraversal) { this.bypassTraversal = bypassTraversal; @@ -65,11 +68,11 @@ public abstract class AbstractLambdaTraversal<S, E> implements Traversal.Admin<S return null == this.bypassTraversal ? new Bytecode() : this.bypassTraversal.getBytecode(); } - @Override public void reset() { if (null != this.bypassTraversal) this.bypassTraversal.reset(); + this.closed = false; } @Override @@ -142,6 +145,7 @@ public abstract class AbstractLambdaTraversal<S, E> implements Traversal.Admin<S @Override public E next() { + if (this.isClosed()) throw FastNoSuchElementException.instance(); if (null != this.bypassTraversal) return this.bypassTraversal.next(); throw new UnsupportedOperationException("The " + this.getClass().getSimpleName() + " can only be used as a predicate traversal"); @@ -156,6 +160,7 @@ public abstract class AbstractLambdaTraversal<S, E> implements Traversal.Admin<S @Override public boolean hasNext() { + if (this.isClosed()) return false; return null == this.bypassTraversal || this.bypassTraversal.hasNext(); } @@ -163,6 +168,10 @@ public abstract class AbstractLambdaTraversal<S, E> implements Traversal.Admin<S public void addStart(final Traverser.Admin<S> start) { if (null != this.bypassTraversal) this.bypassTraversal.addStart(start); + + // match() expects that a traversal that has been iterated can continue to iterate if new starts are + // added therefore the closed state must be reset. + this.closed = false; } @Override @@ -171,6 +180,16 @@ public abstract class AbstractLambdaTraversal<S, E> implements Traversal.Admin<S } /** + * The closed state is not changed by default in this implementation (i.e. the {@link #notifyClose()} method is + * not implemented to set {@link #closed} to {@code true}). It is up to extending classes to implement the + * {@link #notifyClose()} method as needed. + */ + @Override + public boolean isClosed() { + return null == this.bypassTraversal ? this.closed : this.bypassTraversal.isClosed(); + } + + /** * Implementations of this class can never be a root-level traversal as they are specialized implementations * intended to be child traversals by design. */ diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java index 278f467..87acb07 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java @@ -327,9 +327,7 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> { return this.locked; } - /** - * Determines if the traversal has been fully iterated and resources released. - */ + @Override public boolean isClosed() { return closed; } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java index d3f6215..e491979 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java @@ -175,4 +175,12 @@ public final class EmptyTraversal<S, E> implements Traversal.Admin<S, E> { public void setGraph(final Graph graph) { } + + /** + * As it is empty, the traversal is always closed. + */ + @Override + public boolean isClosed() { + return true; + } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java index 05b1f6a..e7738a3 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java @@ -281,7 +281,8 @@ public class TraversalTest { steps = Collections.singletonList(mockEndStep); } - boolean isClosed() { + @Override + public boolean isClosed() { return mockEndStep instanceof MockCloseStep && ((MockCloseStep) mockEndStep).isClosed(); } diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversal.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversal.java index 54e7cc8..bd2b208 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversal.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/remote/DriverRemoteTraversal.java @@ -28,10 +28,12 @@ import org.apache.tinkerpop.gremlin.process.remote.traversal.step.map.RemoteStep import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.EmptyTraverser; +import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Graph; import org.apache.tinkerpop.gremlin.structure.Property; import org.apache.tinkerpop.gremlin.structure.util.Attachable; +import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; import java.util.Iterator; import java.util.Optional; @@ -51,6 +53,7 @@ public class DriverRemoteTraversal<S, E> extends AbstractRemoteTraversal<S, E> { private final Iterator<Traverser.Admin<E>> traversers; private Traverser.Admin<E> lastTraverser = EmptyTraverser.instance(); + private boolean closed = false; public DriverRemoteTraversal(final ResultSet rs, final Client client, final boolean attach, final Optional<Configuration> conf) { // attaching is really just for testing purposes. it doesn't make sense in any real-world scenario as it would @@ -67,11 +70,16 @@ public class DriverRemoteTraversal<S, E> extends AbstractRemoteTraversal<S, E> { @Override public boolean hasNext() { + // if the traversal is closed then resources are released and there is nothing else to iterate + if (this.isClosed()) return false; return this.lastTraverser.bulk() > 0L || this.traversers.hasNext(); } @Override public E next() { + // if the traversal is closed then resources are released and there is nothing else to iterate + if (this.isClosed()) throw FastNoSuchElementException.instance(); + if (0L == this.lastTraverser.bulk()) this.lastTraverser = this.traversers.next(); if (1L == this.lastTraverser.bulk()) { @@ -97,6 +105,17 @@ public class DriverRemoteTraversal<S, E> extends AbstractRemoteTraversal<S, E> { } } + @Override + public boolean isClosed() { + return closed; + } + + @Override + public void notifyClose() { + CloseableIterator.closeIterator(traversers); + this.closed = true; + } + static class TraverserIterator<E> implements Iterator<Traverser.Admin<E>> { private final Iterator<Result> inner;
