[jira] [Commented] (TINKERPOP-1830) Race condition in Tinkergraph index creation

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1830?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16248724#comment-16248724
 ] 

ASF GitHub Bot commented on TINKERPOP-1830:
---

Github user mpollmeier commented on the issue:

https://github.com/apache/tinkerpop/pull/745
  
Instead of marking them as final I simply dropped the assignment. 
Re tp32: yes, I also mentioned this in the description of the ticket. I've 
just squashed all commits and rebased onto tp32


> Race condition in Tinkergraph index creation 
> -
>
> Key: TINKERPOP-1830
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1830
> Project: TinkerPop
>  Issue Type: Bug
>  Components: tinkergraph
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Michael Pollmeier
>Priority: Minor
> Fix For: 3.2.7, 3.3.1
>
>
> My colleage Fabian Yamaguchi  discovered a race condition 
> in tinkergraph's index creation. He fixed it by simply replacing 
> `parallelStream` with `stream`. Quoting his analysis:
> > So, reading the code, you see that this.put is called in parallel, but that 
> > method seems to contain a race as get is called on the index, checked for 
> > null, and a subsequent write is performed. It still seems like using stream 
> > here fixes the problem we've been seeing, and the performance hit is not 
> > significant.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex

[mpollmeier]

Github user mpollmeier commented on the issue:

https://github.com/apache/tinkerpop/pull/745
  
Instead of marking them as final I simply dropped the assignment. 
Re tp32: yes, I also mentioned this in the description of the ticket. I've 
just squashed all commits and rebased onto tp32


---

Re: CountStrategy and TraversalHelper.replaceStep

[pieter gmail]

Created a jira  
for this.

I tested your fix and all tests are passing again. Sqlg and TinkerGraph.

Thanks
Pieter

On 30/10/2017 18:01, Daniel Kuppitz wrote:

Ah, yea, I see what you mean. It's actually replaceStep() which is buggy,
not the strategy. The fix is easy, we just need to change the order of the
2 statements in replaceStep():

public static  void replaceStep(final Step removeStep,
final Step insertStep, final Traversal.Admin traversal) {
 final int i;traversal.removeStep(i = stepIndex(removeStep,
traversal));traversal.addStep(i, insertStep);}


Cheers,
Daniel

On Mon, Oct 30, 2017 at 7:43 AM, pieter gmail 
wrote:


I am on the latest 3.3.1-SNAPSHOT, just pulled master again.

The actual traversals and results are correct. However after the
CountStrategy the VertexStep(OUT,edge) previousStep pointer
(AbstractStep.previousStep) is incorrect. I should be EmptyStep but infact
points to the newly inserted NotStep.

toString() on a traversal does not show the previous/nextStep so you can
not see what I am talking about in the console. This is not breaking
anything in TinkerGraph but breaks stuff in Sqlg.

In CountStrategy if I change line 153 to then my problems go away and
TinkerGraph also works as expected.

//TraversalHelper.replaceStep(prev, new NotStep<>(traversal, inner),
traversal);
NotStep notStep = new NotStep<>(traversal, inner);
TraversalHelper.replaceStep(prev, notStep, traversal);
List notStepTraversal = notStep.getLocalChildren();
Traversal.Admin notStepTraversalFirstStep = notStepTraversal.get(0);
//The first step is pointing to the NotStep, it should point to an
EmptyStep
notStepTraversalFirstStep.getSteps().get(0).setPreviousStep(
EmptyStep.instance());

So I suppose the question is, is it correct for the previousStep of the
first step of a local traversal to point to the parent step and not an
EmptyStep?

The TraversalHelper.replaceStep always makes the first step of the
traversal point to the newly inserted step. If the traversal however is a
local traversal then the root step should be an EmptyStep.

Hope it makes some sense.

Thanks
Pieter



On 30/10/2017 15:28, Daniel Kuppitz wrote:


I don't see any issues. Which version are you talking about?

*gremlin> Gremlin.version()*
*==>3.2.7-SNAPSHOT*
gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
gremlin> g.V(1).repeat(out()).until(__.not(outE())).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin> g.V(1).repeat(out()).until(outE().count().is(0)).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin>
g.V(1).repeat(out()).until(__.not(outE())).values('name').it
erate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]
gremlin>
g.V(1).repeat(out()).until(outE().count().is(0)).values('nam
e').iterate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]


*gremlin> Gremlin.version()*
*==>3.3.1-SNAPSHOT*

gremlin> g = TinkerFactory.createModern().traversal()
==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
gremlin> g.V(1).repeat(out()).until(__.not(outE())).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin> g.V(1).repeat(out()).until(outE().count().is(0)).values('name')
==>lop
==>vadas
==>ripple
==>lop
gremlin>
g.V(1).repeat(out()).until(__.not(outE())).values('name').it
erate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]
gremlin>
g.V(1).repeat(out()).until(outE().count().is(0)).values('nam
e').iterate().toString()
==>[TinkerGraphStep(vertex,[1]), RepeatStep([VertexStep(OUT,vertex),
RepeatEndStep],until([NotStep([VertexStep(OUT,edge)])]),emit(false)),
PropertiesStep([name],value)]


Cheers,
Daniel


On Mon, Oct 30, 2017 at 1:53 AM, pieter gmail 
wrote:

Hi,

Whilst optimizing the NotStep I came across what looks to me like a bug
in
TraversalHelper.replaceStep or perhaps rather in CountStrategy.

The test below shows the bug.

  @Test
  public void g_VX1X_repeatXoutX_untilXoutE_count_isX0XX_name() {
  Graph graph = TinkerFactory.createModern();
  final Traversal traversal1 = graph.traversal()
  .V(convertToVertexId(graph, "marko"))
  .repeat(__.out())
  .until(__.not(__.outE()))
  .values("name");
  checkResults(Arrays.asList("lop", "lop", "ripple", "vadas"),
traversal1);

  List vertexSteps = TraversalHelper.getStepsOfAssi
gnableClassRecursively(VertexStep.class, 

[jira] [Created] (TINKERPOP-1832) TraversalHelper.replaceStep sets previousStep to the wrong step

[pieter martin (JIRA)]

pieter martin created TINKERPOP-1832:


 Summary: TraversalHelper.replaceStep sets previousStep to the 
wrong step
 Key: TINKERPOP-1832
 URL: https://issues.apache.org/jira/browse/TINKERPOP-1832
 Project: TinkerPop
  Issue Type: Bug
  Components: process
Affects Versions: 3.3.0
Reporter: pieter martin


The bug is described over 
[here|http://mail-archives.apache.org/mod_mbox/tinkerpop-dev/201710.mbox/browser]

I have tested [~dkuppitz] fix for it and all seems well.

{code:java}
public static  void replaceStep(final Step removeStep, final 
Step insertStep, final Traversal.Admin traversal) {
final int i;
traversal.removeStep(i = stepIndex(removeStep, traversal));
traversal.addStep(i, insertStep);
}
{code}




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1830) Race condition in Tinkergraph index creation

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1830?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16248449#comment-16248449
 ] 

ASF GitHub Bot commented on TINKERPOP-1830:
---

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/745#discussion_r150381086
  
--- Diff: 
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerIndex.java
 ---
@@ -48,17 +48,18 @@ public TinkerIndex(final TinkerGraph graph, final 
Class indexClass) {
 
 protected void put(final String key, final Object value, final T 
element) {
 Map keyMap = this.index.get(key);
-if (keyMap == null) {
-keyMap = new ConcurrentHashMap<>();
-this.index.put(key, keyMap);
+if (null == keyMap) {
+Map tmpMap = new ConcurrentHashMap<>();
+this.index.putIfAbsent(key, tmpMap);
+keyMap = this.index.get(key);
 }
 Set objects = keyMap.get(value);
 if (null == objects) {
-objects = new HashSet<>();
-keyMap.put(value, objects);
+Set tmpObjects = ConcurrentHashMap.newKeySet();
--- End diff --

can you add a `final` here please?


> Race condition in Tinkergraph index creation 
> -
>
> Key: TINKERPOP-1830
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1830
> Project: TinkerPop
>  Issue Type: Bug
>  Components: tinkergraph
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Michael Pollmeier
>Priority: Minor
> Fix For: 3.2.7, 3.3.1
>
>
> My colleage Fabian Yamaguchi  discovered a race condition 
> in tinkergraph's index creation. He fixed it by simply replacing 
> `parallelStream` with `stream`. Quoting his analysis:
> > So, reading the code, you see that this.put is called in parallel, but that 
> > method seems to contain a race as get is called on the index, checked for 
> > null, and a subsequent write is performed. It still seems like using stream 
> > here fixes the problem we've been seeing, and the performance hit is not 
> > significant.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1830) Race condition in Tinkergraph index creation

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1830?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16248450#comment-16248450
 ] 

ASF GitHub Bot commented on TINKERPOP-1830:
---

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/745
  
Please also include an update to CHANGELOG for this fix.


> Race condition in Tinkergraph index creation 
> -
>
> Key: TINKERPOP-1830
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1830
> Project: TinkerPop
>  Issue Type: Bug
>  Components: tinkergraph
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Michael Pollmeier
>Priority: Minor
> Fix For: 3.2.7, 3.3.1
>
>
> My colleage Fabian Yamaguchi  discovered a race condition 
> in tinkergraph's index creation. He fixed it by simply replacing 
> `parallelStream` with `stream`. Quoting his analysis:
> > So, reading the code, you see that this.put is called in parallel, but that 
> > method seems to contain a race as get is called on the index, checked for 
> > null, and a subsequent write is performed. It still seems like using stream 
> > here fixes the problem we've been seeing, and the performance hit is not 
> > significant.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TINKERPOP-1830) Race condition in Tinkergraph index creation

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TINKERPOP-1830?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16248448#comment-16248448
 ] 

ASF GitHub Bot commented on TINKERPOP-1830:
---

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/745#discussion_r150381081
  
--- Diff: 
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerIndex.java
 ---
@@ -48,17 +48,18 @@ public TinkerIndex(final TinkerGraph graph, final 
Class indexClass) {
 
 protected void put(final String key, final Object value, final T 
element) {
 Map keyMap = this.index.get(key);
-if (keyMap == null) {
-keyMap = new ConcurrentHashMap<>();
-this.index.put(key, keyMap);
+if (null == keyMap) {
+Map tmpMap = new ConcurrentHashMap<>();
--- End diff --

can you add a `final` here please?


> Race condition in Tinkergraph index creation 
> -
>
> Key: TINKERPOP-1830
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1830
> Project: TinkerPop
>  Issue Type: Bug
>  Components: tinkergraph
>Affects Versions: 3.3.0, 3.2.6
>Reporter: Michael Pollmeier
>Priority: Minor
> Fix For: 3.2.7, 3.3.1
>
>
> My colleage Fabian Yamaguchi  discovered a race condition 
> in tinkergraph's index creation. He fixed it by simply replacing 
> `parallelStream` with `stream`. Quoting his analysis:
> > So, reading the code, you see that this.put is called in parallel, but that 
> > method seems to contain a race as get is called on the index, checked for 
> > null, and a subsequent write is performed. It still seems like using stream 
> > here fixes the problem we've been seeing, and the performance hit is not 
> > significant.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex

[spmallette]

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/745
  
Please also include an update to CHANGELOG for this fix.


---

[GitHub] tinkerpop pull request #745: TINKERPOP-1830: fix race condition in TinkerInd...

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/745#discussion_r150381086
  
--- Diff: 
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerIndex.java
 ---
@@ -48,17 +48,18 @@ public TinkerIndex(final TinkerGraph graph, final 
Class indexClass) {
 
 protected void put(final String key, final Object value, final T 
element) {
 Map keyMap = this.index.get(key);
-if (keyMap == null) {
-keyMap = new ConcurrentHashMap<>();
-this.index.put(key, keyMap);
+if (null == keyMap) {
+Map tmpMap = new ConcurrentHashMap<>();
+this.index.putIfAbsent(key, tmpMap);
+keyMap = this.index.get(key);
 }
 Set objects = keyMap.get(value);
 if (null == objects) {
-objects = new HashSet<>();
-keyMap.put(value, objects);
+Set tmpObjects = ConcurrentHashMap.newKeySet();
--- End diff --

can you add a `final` here please?


---

[GitHub] tinkerpop pull request #745: TINKERPOP-1830: fix race condition in TinkerInd...

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/745#discussion_r150381081
  
--- Diff: 
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerIndex.java
 ---
@@ -48,17 +48,18 @@ public TinkerIndex(final TinkerGraph graph, final 
Class indexClass) {
 
 protected void put(final String key, final Object value, final T 
element) {
 Map keyMap = this.index.get(key);
-if (keyMap == null) {
-keyMap = new ConcurrentHashMap<>();
-this.index.put(key, keyMap);
+if (null == keyMap) {
+Map tmpMap = new ConcurrentHashMap<>();
--- End diff --

can you add a `final` here please?


---