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 d31cd6cf251810ccca2643d9a97878b642cef91b Author: Stephen Mallette <[email protected]> AuthorDate: Tue Dec 21 08:37:18 2021 -0500 TINKERPOP-2626 Added explicit closed state to DefaultTraversal The closed state prevents futher iteration on a traversal when it has been exhausted and resources have been released. This change is non-breaking but should likely be refactored when merged to branch that can take a breaking change. --- CHANGELOG.asciidoc | 1 + .../gremlin/process/traversal/Traversal.java | 14 +++++++- .../process/traversal/util/DefaultTraversal.java | 38 ++++++++++++++++++++-- 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index ed87ae9..f2df59e 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -24,6 +24,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima === TinkerPop 3.4.13 (Release Date: NOT OFFICIALLY RELEASED YET) * Fixed `RangeGlobalStep` which was prematurely closing the iterator. +* Added explicit state to `DefaultTraversal` to track whether or not it was fully iterated and closed to ensure it released resources properly. * Prevented XML External Entity (XXE) style attacks via `GraphMLReader` by disabling DTD and external entities by default. * Improved error message for failed serialization for HTTP-based requests. * Fixed a `NullPointerException` that could occur during a failed `Connection` initialization due to uninstantiated `AtomicInteger`. 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 b1b45f3..0284fff 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 @@ -285,7 +285,8 @@ public interface Traversal<S, E> extends Iterator<E>, Serializable, Cloneable, A } /** - * Releases resources opened in any steps that implement {@link AutoCloseable}. + * Releases resources opened in any steps that implement {@link AutoCloseable}. If this method is overridden,the + * implementer should invoke {@link #notifyClose()}. */ @Override public default void close() throws Exception { @@ -293,6 +294,17 @@ public interface Traversal<S, E> extends Iterator<E>, Serializable, Cloneable, A if (step instanceof AutoCloseable) ((AutoCloseable) step).close(); } + + notifyClose(); + } + + /** + * Gets a callback from {@link #close()} for additional operations specific to the {@link Traversal} implementation. + * A good implementation will use {@link #close()} to release resources in steps and this method to release + * resources specific to the {@link Traversal} implementations. + */ + public default void notifyClose() { + // do nothing by default } /** 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 3aa7bc1..83a06f7 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 @@ -65,6 +65,7 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> { protected Set<TraverserRequirement> requirements; protected boolean locked = false; + protected boolean closed = false; protected Bytecode bytecode; private DefaultTraversal(final Graph graph, final TraversalStrategies traversalStrategies, final Bytecode bytecode) { @@ -192,6 +193,9 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<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; + if (!this.locked) this.applyStrategies(); final boolean more = this.lastTraverser.bulk() > 0L || this.finalEndStep.hasNext(); if (!more) CloseableIterator.closeIterator(this); @@ -200,6 +204,10 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> { @Override public E next() { + // if the traversal is closed then resources are released and there is nothing else to iterate + if (this.isClosed()) + throw this.parent instanceof EmptyStep ? new NoSuchElementException() : FastNoSuchElementException.instance(); + try { if (!this.locked) this.applyStrategies(); if (this.lastTraverser.bulk() == 0L) @@ -220,18 +228,32 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> { this.steps.forEach(Step::reset); this.finalEndStep.reset(); this.lastTraverser = EmptyTraverser.instance(); + this.closed = false; } @Override public void addStart(final Traverser.Admin<S> start) { if (!this.locked) this.applyStrategies(); - if (!this.steps.isEmpty()) this.steps.get(0).addStart(start); + if (!this.steps.isEmpty()) { + this.steps.get(0).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 public void addStarts(final Iterator<Traverser.Admin<S>> starts) { if (!this.locked) this.applyStrategies(); - if (!this.steps.isEmpty()) this.steps.get(0).addStarts(starts); + + if (!this.steps.isEmpty()) { + this.steps.get(0).addStarts(starts); + + // 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 @@ -279,6 +301,18 @@ public class DefaultTraversal<S, E> implements Traversal.Admin<S, E> { return this.locked; } + /** + * Determines if the traversal has been fully iterated and resources released. + */ + public boolean isClosed() { + return closed; + } + + @Override + public void notifyClose() { + this.closed = true; + } + @Override public void setSideEffects(final TraversalSideEffects sideEffects) { this.sideEffects = sideEffects;
