You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2021/11/19 15:44:06 UTC
[GitHub] [tinkerpop] spmallette opened a new pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
spmallette opened a new pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502
https://issues.apache.org/jira/browse/TINKERPOP-2626
This is not a complete fix for this issue but does get rid of the premature close. Opted to try to close the Traversal as part of hasNext() as in a sense it is a way to complete iteration of the traversal. Without that change certain tests begin to fail so some additional change besides removing the premature close had to be added.
All tests pass with `docker/build.sh -t -n -i`
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502#issuecomment-983549704
I thought of a bug in this implementation.
When we call `hasNext()` first time and there are no more results, we will close the traversal. `hasNext()` is supposed to be an idempotent operation. If we call `hasNext()` again on this closed traversal, we may encounter unexpected behaviour because we are again trying to find a result on a traversal that has already been closed (and the underlying storage implementation might have released resources).
The fix would be to add a lifecycle state in Traversal.java. Set the lifecycle state to closed when close() is called. Once it is closed, any call to hasNext should return false and any call to next() should return NoSuchElement() exception.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502#discussion_r756232393
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java
##########
@@ -193,7 +193,9 @@ public void applyStrategies() throws IllegalStateException {
@Override
public boolean hasNext() {
if (!this.locked) this.applyStrategies();
- return this.lastTraverser.bulk() > 0L || this.finalEndStep.hasNext();
+ final boolean more = this.lastTraverser.bulk() > 0L || this.finalEndStep.hasNext();
+ if (!more) CloseableIterator.closeIterator(this);
Review comment:
the other terminal steps tend to call `fill()` which performs a close properly.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502#issuecomment-998714711
I had tried a similar approach - I stopped when i noticed around item 3 that `isClosed()` could only work nicely on an implementation like `DefaultTraversal`. As there are other `Traversal` implementations to consider putting it where it ideally goes as a member of `Traversal` won't come without a breaking change which is why i asked for your thoughts.
I can implement this in 3.5.2 as you describe it and then take a breaking change in 3.6.0 after this merges to `master` to promote `isClosed()` to `Traversal`.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502#issuecomment-993074844
after looking at this a bit, i'm not sure i see how we can get this change in without some form of breaking change to the `Traversal` API. @divijvaidya maybe i'm not envisioning the change as you were seeing it happen...thoughts?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502#issuecomment-977866882
LGTM. Vote +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502#discussion_r756166726
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java
##########
@@ -193,7 +193,9 @@ public void applyStrategies() throws IllegalStateException {
@Override
public boolean hasNext() {
if (!this.locked) this.applyStrategies();
- return this.lastTraverser.bulk() > 0L || this.finalEndStep.hasNext();
+ final boolean more = this.lastTraverser.bulk() > 0L || this.finalEndStep.hasNext();
+ if (!more) CloseableIterator.closeIterator(this);
Review comment:
Just a comprehension question: Why does it make sense to close the iterator for `hasNext()`, but not do the same for the other terminal steps?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette merged pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on pull request #1502: TINKERPOP-2626 Prevent premature close of traversal
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1502:
URL: https://github.com/apache/tinkerpop/pull/1502#issuecomment-997810991
> after looking at this a bit, i'm not sure i see how we can get this change in without some form of breaking change to the `Traversal` API. @divijvaidya maybe i'm not envisioning the change as you were seeing it happen...thoughts?
I am proposing the following changes, let me know if they sound good to you.
1. Add an abstract method to `Traversal.java` called `notifyClosed()`
2. That method should be called inside `close()` method of `Traversal.java`
3. `DefaultTraversal.java` should have a member called `isClosed` which would be set to `true` inside it's implementation of `notifyClosed()`. `DefaultTraversal` would also have a method called `isClosed()` which will return the value of `isClosed` member.
4. In the `hasNext()` method of `DefaultTraversal`, it will check if `isClosed()` is true and if yes, then it will return false. Note that this is not breaking in the sense that today the behaviour of iterator hasNext() after being closed in undefined. We are correcting the semantics here such that you cannot access the iterator when it is closed already. Same change for `next()`, it should return NoSuchElement exception when called on closed iterator.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org