[
https://issues.apache.org/jira/browse/TINKERPOP-3283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18121660#comment-18121660
]
ASF GitHub Bot commented on TINKERPOP-3283:
-------------------------------------------
Cole-Greer commented on PR #3688:
URL: https://github.com/apache/tinkerpop/pull/3688#issuecomment-5937520996
> Thanks for fixing this issue, the code changes here look good to me. I
think there is value in adding some quick extra docs and tests for this as
well. Could you add a quick note in the `considerations` and `exceptions`
sections of the [repeat semantics
docs](https://github.com/apache/tinkerpop/blob/11cc6bfb512451ccca76a872076df0fdcbd01bd2/docs/src/dev/provider/gremlin-semantics.asciidoc?plain=1#L1421)
explaining the iteration cap and exception?
>
> Additionally, I think it would be good to add some trivial scenarios to
[repeat.feature](https://github.com/apache/tinkerpop/blob/3.7-dev/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/branch/Repeat.feature)
which enforce these semantics. Something like these should do it:
>
> ```
> g.V(1).repeat(loops()).times(Integer.MAX_VALUE)
> ==> Integer.MAX_VALUE-1
>
> g.V(1).repeat(loops()).times(Integer.MAX_VALUE+1)
> ==> Exception
> ```
>
> There's already a few examples of how to assert exceptions in
repeat.feature which you can copy from.
If these tests are too slow, it's probably good enough to just confirm the
loops counter can exceed short max:
```
g.V(1).repeat(loops()).times(100000)
==> 99999
```
> loops() quietly overflows
> -------------------------
>
> Key: TINKERPOP-3283
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3283
> Project: TinkerPop
> Issue Type: Bug
> Components: process
> Affects Versions: 3.7.6, 3.8.1
> Reporter: Cole Greer
> Priority: Major
>
> The `loops()` step is ultimately backed by a `LabelledCounter`, which tracks
> the loop count as a `short`
> (https://github.com/apache/tinkerpop/blob/519a7be1576c6ccc7f028382c108f02933b1140b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/util/LabelledCounter.java#L30).
> There is no overflow protection on this count, which can lead to some
> undesirable behaviours around the boundary of 32767 (short max):
> {code:java}
> gremlin> g.inject(0).repeat(loops()).emit().times(32767).count()
> ==>32767
> gremlin> g.inject(0).repeat(loops()).emit().times(32768).count()
> //hung, as the loop counter overflows before reaching 32768
> gremlin> g.inject(0).repeat(addV('TestNode').
> property(id, constant('TID-').concat(loops().asString())).
> property('rid', loops()).
> property('series','M').
> property('uuid', 'ab0ae056-5d59-48df-a582-92c7c1bb9fcc')).
> times(80000).iterate()
> Vertex with id already exists: TID-0
> Type ':help' or ':h' for help.
> Display stack trace? [yN]n
> gremlin> g.V()
> ==>v[TID--25318] // writes vertices with negative value ids after loops()
> overflows
> ==>v[TID--25319]
> ==>v[TID--15991]
> ==>v[TID--15992]
> ==>v[TID--15990]
> ==>v[TID--25310]
> {code}
> The silent overflow leads to really unintuitive results and should be
> considered a bug. This could be solved by adding some smart overflow
> detection (either resulting in a clear exception or expansion to a new
> long-counting traverser), or we could simply globally expand the counter to
> be a long, and suffer the impact from the increased memory consumption. These
> options should each be evaluated before implementing any fix.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
