Skip to content

Commit

Permalink
Merge pull request #869 from tinwelint/rel-load-problem
Browse files Browse the repository at this point in the history 
Fixes a race condition where more than one thread loads the relationship...
  • Loading branch information
@johan-neo
johan-neo committed Sep 21, 2012
2 parents fc8c53c + 66d7d77 commit 152b6ed
Show file tree
Hide file tree
Showing 3 changed files with 219 additions and 37 deletions.
14 changes: 8 additions & 6 deletions kernel/src/main/java/org/neo4j/kernel/impl/core/IntArrayIterator.java
View file
Expand Up @@ -29,6 +29,7 @@
import org.neo4j.graphdb.Relationship;
import org.neo4j.graphdb.RelationshipType;
import org.neo4j.helpers.collection.PrefetchingIterator;
import org.neo4j.kernel.impl.core.NodeImpl.LoadStatus;
import org.neo4j.kernel.impl.util.RelIdArray;
import org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper;
import org.neo4j.kernel.impl.util.RelIdIterator;
Expand All @@ -44,14 +45,14 @@ class IntArrayIterator extends PrefetchingIterator<Relationship> implements Iter
private final List<RelIdIterator> rels;

// This is just for optimization
private boolean isFullyLoaded;
private boolean lastTimeILookedThereWasMoreToLoad;

IntArrayIterator( List<RelIdIterator> rels, NodeImpl fromNode,
DirectionWrapper direction, NodeManager nodeManager, RelationshipType[] types,
boolean isFullyLoaded )
boolean hasMoreToLoad )
{
this.rels = rels;
this.isFullyLoaded = isFullyLoaded;
this.lastTimeILookedThereWasMoreToLoad = hasMoreToLoad;
this.typeIterator = rels.iterator();
this.currentTypeIterator = typeIterator.hasNext() ? typeIterator.next() : RelIdArray.EMPTY.iterator( direction );
this.fromNode = fromNode;
Expand Down Expand Up @@ -82,20 +83,22 @@ protected Relationship fetchNextOrNull()
}
}

LoadStatus status;
while ( !currentTypeIterator.hasNext() )
{
if ( typeIterator.hasNext() )
{
currentTypeIterator = typeIterator.next();
}
else if ( fromNode.getMoreRelationships( nodeManager ) ||
else if ( (status = fromNode.getMoreRelationships( nodeManager )).loaded()
// This is here to guard for that someone else might have loaded
// stuff in this relationship chain (and exhausted it) while I
// iterated over my batch of relationships. It will only happen
// for nodes which have more than <grab size> relationships and
// isn't fully loaded when starting iterating.
!isFullyLoaded )
|| lastTimeILookedThereWasMoreToLoad )
{
lastTimeILookedThereWasMoreToLoad = status.hasMoreToLoad();
Map<String,RelIdIterator> newRels = new HashMap<String,RelIdIterator>();
for ( RelIdIterator itr : rels )
{
Expand Down Expand Up @@ -138,7 +141,6 @@ else if ( fromNode.getMoreRelationships( nodeManager ) ||

typeIterator = rels.iterator();
currentTypeIterator = typeIterator.hasNext() ? typeIterator.next() : RelIdArray.EMPTY.iterator( direction );
isFullyLoaded = !fromNode.hasMoreRelationshipsToLoad();
}
else
{
Expand Down
96 changes: 65 additions & 31 deletions kernel/src/main/java/org/neo4j/kernel/impl/core/NodeImpl.java
View file
Expand Up @@ -21,6 +21,7 @@

import static org.neo4j.kernel.impl.cache.SizeOfs.withArrayOverheadIncludingReferences;
import static org.neo4j.kernel.impl.util.RelIdArray.empty;
import static org.neo4j.kernel.impl.util.RelIdArray.wrap;

import java.util.Arrays;
import java.util.Collection;
Expand Down Expand Up @@ -54,7 +55,7 @@ public class NodeImpl extends ArrayBasedPrimitive

private volatile RelIdArray[] relationships;

private long relChainPosition = Record.NO_NEXT_RELATIONSHIP.intValue();
private volatile long relChainPosition = Record.NO_NEXT_RELATIONSHIP.intValue();
private final long id;

NodeImpl( long id, long firstRel, long firstProp )
Expand Down Expand Up @@ -132,9 +133,17 @@ protected ArrayMap<Integer, PropertyData> loadProperties(
return nodeManager.loadProperties( this, light );
}

List<RelIdIterator> getAllRelationships( NodeManager nodeManager, DirectionWrapper direction )
Iterable<Relationship> getAllRelationships( NodeManager nodeManager, DirectionWrapper direction )
{
ensureRelationshipMapNotNull( nodeManager );

// We need to check if there are more relationships to load before grabbing
// the references to the RelIdArrays since otherwise there could be
// another concurrent thread exhausting the chain position in between the point
// where we got an empty iterator for a type that the other thread loaded and
// the point where we check whether or not there are more relationships to load.
boolean hasMore = hasMoreRelationshipsToLoad();

List<RelIdIterator> relTypeList = new LinkedList<RelIdIterator>();
boolean hasModifications = nodeManager.getLockReleaser().hasRelationshipModifications( this );
ArrayMap<String,RelIdArray> addMap = null;
Expand Down Expand Up @@ -176,13 +185,22 @@ List<RelIdIterator> getAllRelationships( NodeManager nodeManager, DirectionWrapp
}
}
}
return relTypeList;

return new IntArrayIterator( relTypeList, this, direction, nodeManager, new RelationshipType[0], hasMore );
}

List<RelIdIterator> getAllRelationshipsOfType( NodeManager nodeManager,
Iterable<Relationship> getAllRelationshipsOfType( NodeManager nodeManager,
DirectionWrapper direction, RelationshipType... types)
{
ensureRelationshipMapNotNull( nodeManager );

// We need to check if there are more relationships to load before grabbing
// the references to the RelIdArrays since otherwise there could be
// another concurrent thread exhausting the chain position in between the point
// where we got an empty iterator for a type that the other thread loaded and
// the point where we check whether or not there are more relationships to load.
boolean hasMore = hasMoreRelationshipsToLoad();

List<RelIdIterator> relTypeList = new LinkedList<RelIdIterator>();
boolean hasModifications = nodeManager.getLockReleaser().hasRelationshipModifications( this );
for ( RelationshipType type : types )
Expand All @@ -204,51 +222,42 @@ List<RelIdIterator> getAllRelationshipsOfType( NodeManager nodeManager,
}
relTypeList.add( iterator );
}
return relTypeList;

return new IntArrayIterator( relTypeList, this, direction, nodeManager, types, hasMore );
}

public Iterable<Relationship> getRelationships( NodeManager nodeManager )
{
return new IntArrayIterator( getAllRelationships( nodeManager, DirectionWrapper.BOTH ), this,
DirectionWrapper.BOTH, nodeManager, new RelationshipType[0], !hasMoreRelationshipsToLoad() );
return getAllRelationships( nodeManager, DirectionWrapper.BOTH );
}

public Iterable<Relationship> getRelationships( NodeManager nodeManager, Direction dir )
{
DirectionWrapper direction = RelIdArray.wrap( dir );
return new IntArrayIterator( getAllRelationships( nodeManager, direction ), this, direction,
nodeManager, new RelationshipType[0], !hasMoreRelationshipsToLoad() );
return getAllRelationships( nodeManager, wrap( dir ) );
}

public Iterable<Relationship> getRelationships( NodeManager nodeManager, RelationshipType type )
{
RelationshipType types[] = new RelationshipType[] { type };
return new IntArrayIterator( getAllRelationshipsOfType( nodeManager, DirectionWrapper.BOTH, types ),
this, DirectionWrapper.BOTH, nodeManager, types, !hasMoreRelationshipsToLoad() );
return getAllRelationshipsOfType( nodeManager, DirectionWrapper.BOTH, new RelationshipType[] { type } );
}

public Iterable<Relationship> getRelationships( NodeManager nodeManager,
RelationshipType... types )
{
return new IntArrayIterator( getAllRelationshipsOfType( nodeManager, DirectionWrapper.BOTH, types ),
this, DirectionWrapper.BOTH, nodeManager, types, !hasMoreRelationshipsToLoad() );
return getAllRelationshipsOfType( nodeManager, DirectionWrapper.BOTH, types );
}

public Iterable<Relationship> getRelationships( NodeManager nodeManager,
Direction direction, RelationshipType... types )
{
DirectionWrapper dir = RelIdArray.wrap( direction );
return new IntArrayIterator( getAllRelationshipsOfType( nodeManager, dir, types ),
this, dir, nodeManager, types, !hasMoreRelationshipsToLoad() );
return getAllRelationshipsOfType( nodeManager, wrap( direction ), types );
}

public Relationship getSingleRelationship( NodeManager nodeManager, RelationshipType type,
Direction dir )
{
DirectionWrapper direction = RelIdArray.wrap( dir );
RelationshipType types[] = new RelationshipType[] { type };
Iterator<Relationship> rels = new IntArrayIterator( getAllRelationshipsOfType( nodeManager,
direction, types ), this, direction, nodeManager, types, !hasMoreRelationshipsToLoad() );
Iterator<Relationship> rels = getAllRelationshipsOfType( nodeManager, wrap( dir ),
new RelationshipType[] { type } ).iterator();
if ( !rels.hasNext() )
{
return null;
Expand All @@ -265,10 +274,7 @@ public Relationship getSingleRelationship( NodeManager nodeManager, Relationship
public Iterable<Relationship> getRelationships( NodeManager nodeManager, RelationshipType type,
Direction dir )
{
RelationshipType types[] = new RelationshipType[] { type };
DirectionWrapper direction = RelIdArray.wrap( dir );
return new IntArrayIterator( getAllRelationshipsOfType( nodeManager, direction, types ),
this, direction, nodeManager, types, !hasMoreRelationshipsToLoad() );
return getAllRelationshipsOfType( nodeManager, wrap( dir ), new RelationshipType[] { type } );
}

public void delete( NodeManager nodeManager, Node proxy )
Expand Down Expand Up @@ -439,25 +445,52 @@ boolean hasMoreRelationshipsToLoad()
{
return getRelChainPosition() != Record.NO_NEXT_RELATIONSHIP.intValue();
}

static enum LoadStatus
{
NOTHING( false, false ),
LOADED_END( true, false ),
LOADED_MORE( true, true );

private final boolean loaded;
private final boolean more;

private LoadStatus( boolean loaded, boolean more )
{
this.loaded = loaded;
this.more = more;
}

public boolean loaded()
{
return this.loaded;
}

public boolean hasMoreToLoad()
{
return this.more;
}
}

boolean getMoreRelationships( NodeManager nodeManager )
LoadStatus getMoreRelationships( NodeManager nodeManager )
{
Triplet<ArrayMap<String,RelIdArray>,List<RelationshipImpl>,Long> rels;
if ( !hasMoreRelationshipsToLoad() )
{
return false;
return LoadStatus.NOTHING;
}
boolean more = false;
synchronized ( this )
{
if ( !hasMoreRelationshipsToLoad() )
{
return false;
return LoadStatus.NOTHING;
}
rels = loadMoreRelationshipsFromNodeManager(nodeManager);
ArrayMap<String,RelIdArray> addMap = rels.first();
if ( addMap.size() == 0 )
{
return false;
return LoadStatus.NOTHING;
}
for ( String type : addMap.keySet() )
{
Expand All @@ -479,10 +512,11 @@ boolean getMoreRelationships( NodeManager nodeManager )
}
}
setRelChainPosition( rels.third() );
more = hasMoreRelationshipsToLoad();
updateSize( nodeManager );
}
nodeManager.putAllInRelCache( rels.second() );
return true;
return more ? LoadStatus.LOADED_MORE : LoadStatus.LOADED_END;
}

private Triplet<ArrayMap<String, RelIdArray>, List<RelationshipImpl>, Long>
Expand Down

0 comments on commit 152b6ed

Please sign in to comment.