You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by mpollmeier <gi...@git.apache.org> on 2017/11/10 01:17:11 UTC
[GitHub] tinkerpop pull request #745: TINKERPOP-1830: fix race condition in TinkerInd...
GitHub user mpollmeier opened a pull request:
https://github.com/apache/tinkerpop/pull/745
TINKERPOP-1830: fix race condition in TinkerIndex
My colleage @fabsx00 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.
Ticket: https://issues.apache.org/jira/browse/TINKERPOP-1830
After initial feedback I can backport this onto tp32.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mpollmeier/tinkerpop mp/1830-tinkergraph-index-race-condition
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/745.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #745
----
commit ca5aa798093b58833bf54d7a495a0928bca41555
Author: Fabian Yamaguchi <fa...@shiftleft.io>
Date: 2017-10-27T16:43:15Z
patch for TinkerIndex
----
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by spmallette <gi...@git.apache.org>.
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 issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by mpollmeier <gi...@git.apache.org>.
Github user mpollmeier commented on the issue:
https://github.com/apache/tinkerpop/pull/745
ok that's done. added a changelog entry, merged this branch into tp32 and merged tp32 into master.
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/745
`mvn -pl tinkergraph-gremlin -DskipIntegrationTests=false -am clean install` build SUCCESS
VOTE +1
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by mpollmeier <gi...@git.apache.org>.
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
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/745
The race condition is in `put()` and it's used elsewhere. It would make more sense to fix `put()` to be thread-safe. Then we can keep `parallelStream()`
```
Map<Object, Set<T>> keyMap = this.index.get(key);
if (keyMap == null) {
keyMap = this.index.putIfAbsent(key, new ConcurrentHashMap<>());
}
Set<T> objects = keyMap.get(value);
if (null == objects) {
objects = keyMap.putIfAbsent(value, ConcurrentHashMap.newKeySet());
}
objects.add(element);
```
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by mpollmeier <gi...@git.apache.org>.
Github user mpollmeier commented on the issue:
https://github.com/apache/tinkerpop/pull/745
I made the same incorrect assumption. Fixed with your latest snippet and tested with our large testbase. No NPEs, no race conditions so far, looks good.
VOTE +1
---
[GitHub] tinkerpop pull request #745: TINKERPOP-1830: fix race condition in TinkerInd...
Posted by spmallette <gi...@git.apache.org>.
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<T> indexClass) {
protected void put(final String key, final Object value, final T element) {
Map<Object, Set<T>> keyMap = this.index.get(key);
- if (keyMap == null) {
- keyMap = new ConcurrentHashMap<>();
- this.index.put(key, keyMap);
+ if (null == keyMap) {
+ Map<Object, Set<T>> tmpMap = new ConcurrentHashMap<>();
--- End diff --
can you add a `final` here please?
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/745
@mpollmeier Sorry, I thought that `putIfAbsent()` returned the current map but it returns old mapping or `null` hence the `NPEs`. Should be:
```java
protected void put(final String key, final Object value, final T element) {
Map<Object, Set<T>> keyMap = this.index.get(key);
if (null == keyMap) {
Map<Object, Set<T>> tmpMap = new ConcurrentHashMap<>();
this.index.putIfAbsent(key, tmpMap);
keyMap = this.index.get(key);
}
Set<T> objects = keyMap.get(value);
if (null == objects) {
Set<T> tmpObjects = ConcurrentHashMap.newKeySet();
keyMap.putIfAbsent(value, tmpObjects);
objects = keyMap.get(value);
}
objects.add(element);
}
```
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by mpollmeier <gi...@git.apache.org>.
Github user mpollmeier commented on the issue:
https://github.com/apache/tinkerpop/pull/745
Agreed, that's a better solution @robertdale. Note that `putIfAbsent` is not generally thread safe, but since we use `ConcurrentHashMap` it should be. I'll amend the commit.
---
[GitHub] tinkerpop pull request #745: TINKERPOP-1830: fix race condition in TinkerInd...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/745
---
[GitHub] tinkerpop issue #745: TINKERPOP-1830: fix race condition in TinkerIndex
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/745
sorry - i missed the `tp32` comment in your description. your changes look fine now.
VOTE +1
---
[GitHub] tinkerpop pull request #745: TINKERPOP-1830: fix race condition in TinkerInd...
Posted by spmallette <gi...@git.apache.org>.
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<T> indexClass) {
protected void put(final String key, final Object value, final T element) {
Map<Object, Set<T>> keyMap = this.index.get(key);
- if (keyMap == null) {
- keyMap = new ConcurrentHashMap<>();
- this.index.put(key, keyMap);
+ if (null == keyMap) {
+ Map<Object, Set<T>> tmpMap = new ConcurrentHashMap<>();
+ this.index.putIfAbsent(key, tmpMap);
+ keyMap = this.index.get(key);
}
Set<T> objects = keyMap.get(value);
if (null == objects) {
- objects = new HashSet<>();
- keyMap.put(value, objects);
+ Set<T> tmpObjects = ConcurrentHashMap.newKeySet();
--- End diff --
can you add a `final` here please?
---