diff --git a/kernel/src/main/java/org/neo4j/kernel/impl/core/IntArrayIterator.java b/kernel/src/main/java/org/neo4j/kernel/impl/core/IntArrayIterator.java index 58bad161b..6e062341a 100644 --- a/kernel/src/main/java/org/neo4j/kernel/impl/core/IntArrayIterator.java +++ b/kernel/src/main/java/org/neo4j/kernel/impl/core/IntArrayIterator.java @@ -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; @@ -44,14 +45,14 @@ class IntArrayIterator extends PrefetchingIterator implements Iter private final List rels; // This is just for optimization - private boolean isFullyLoaded; + private boolean lastTimeILookedThereWasMoreToLoad; IntArrayIterator( List 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; @@ -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 relationships and // isn't fully loaded when starting iterating. - !isFullyLoaded ) + || lastTimeILookedThereWasMoreToLoad ) { + lastTimeILookedThereWasMoreToLoad = status.hasMoreToLoad(); Map newRels = new HashMap(); for ( RelIdIterator itr : rels ) { @@ -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 { diff --git a/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeImpl.java b/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeImpl.java index 17de54fa6..0a94d98c1 100644 --- a/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeImpl.java +++ b/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeImpl.java @@ -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; @@ -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 ) @@ -132,9 +133,17 @@ protected ArrayMap loadProperties( return nodeManager.loadProperties( this, light ); } - List getAllRelationships( NodeManager nodeManager, DirectionWrapper direction ) + Iterable 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 relTypeList = new LinkedList(); boolean hasModifications = nodeManager.getLockReleaser().hasRelationshipModifications( this ); ArrayMap addMap = null; @@ -176,13 +185,22 @@ List getAllRelationships( NodeManager nodeManager, DirectionWrapp } } } - return relTypeList; + + return new IntArrayIterator( relTypeList, this, direction, nodeManager, new RelationshipType[0], hasMore ); } - List getAllRelationshipsOfType( NodeManager nodeManager, + Iterable 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 relTypeList = new LinkedList(); boolean hasModifications = nodeManager.getLockReleaser().hasRelationshipModifications( this ); for ( RelationshipType type : types ) @@ -204,51 +222,42 @@ List getAllRelationshipsOfType( NodeManager nodeManager, } relTypeList.add( iterator ); } - return relTypeList; + + return new IntArrayIterator( relTypeList, this, direction, nodeManager, types, hasMore ); } public Iterable 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 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 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 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 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 rels = new IntArrayIterator( getAllRelationshipsOfType( nodeManager, - direction, types ), this, direction, nodeManager, types, !hasMoreRelationshipsToLoad() ); + Iterator rels = getAllRelationshipsOfType( nodeManager, wrap( dir ), + new RelationshipType[] { type } ).iterator(); if ( !rels.hasNext() ) { return null; @@ -265,10 +274,7 @@ public Relationship getSingleRelationship( NodeManager nodeManager, Relationship public Iterable 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 ) @@ -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,List,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 addMap = rels.first(); if ( addMap.size() == 0 ) { - return false; + return LoadStatus.NOTHING; } for ( String type : addMap.keySet() ) { @@ -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, List, Long> diff --git a/kernel/src/test/java/org/neo4j/kernel/impl/core/TestConcurrentRelationshipChainLoadingIssue.java b/kernel/src/test/java/org/neo4j/kernel/impl/core/TestConcurrentRelationshipChainLoadingIssue.java new file mode 100644 index 000000000..7fdd88bfe --- /dev/null +++ b/kernel/src/test/java/org/neo4j/kernel/impl/core/TestConcurrentRelationshipChainLoadingIssue.java @@ -0,0 +1,146 @@ +/** + * Copyright (c) 2002-2012 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.kernel.impl.core; + +import static java.lang.Runtime.getRuntime; +import static java.lang.System.currentTimeMillis; +import static java.util.concurrent.Executors.newCachedThreadPool; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.Assert.assertEquals; +import static org.neo4j.graphdb.factory.GraphDatabaseSettings.cache_type; +import static org.neo4j.graphdb.factory.GraphDatabaseSettings.relationship_grab_size; +import static org.neo4j.helpers.collection.IteratorUtil.count; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.Rule; +import org.junit.Test; +import org.neo4j.graphdb.Node; +import org.neo4j.graphdb.Transaction; +import org.neo4j.graphdb.factory.GraphDatabaseBuilder; +import org.neo4j.kernel.GraphDatabaseAPI; +import org.neo4j.kernel.impl.MyRelTypes; +import org.neo4j.test.ImpermanentDatabaseRule; + +/** + * This isn't a deterministic test, but instead tries to trigger a race condition + * for a couple of seconds. The original issues is mostly seen immediately, but after + * a fix is in this test will take the full amount of seconds unfortunately. + */ +public class TestConcurrentRelationshipChainLoadingIssue +{ + private final int relCount = 2; + public final @Rule ImpermanentDatabaseRule graphDb = new ImpermanentDatabaseRule() + { + protected void configure( GraphDatabaseBuilder builder ) + { + builder.setConfig( cache_type, "weak" ); + builder.setConfig( relationship_grab_size, "" + (relCount/2) ); + } + }; + + @Test + public void tryToTriggerRelationshipLoadingStoppingMidWay() throws Throwable + { + GraphDatabaseAPI db = graphDb.getGraphDatabaseAPI(); + final Node node = createNodeWithRelationships( db ); + + long end = currentTimeMillis()+SECONDS.toMillis( 5 ); + while ( currentTimeMillis() < end ) + tryOnce( db, node ); + } + + private void awaitStartSignalAndRandomTimeLonger( final CountDownLatch startSignal ) + { + try + { + startSignal.await(); + idleLoop( (int) (System.currentTimeMillis()%100000) ); + } + catch ( InterruptedException e ) + { + throw new RuntimeException( e ); + } + } + + private void tryOnce( final GraphDatabaseAPI db, final Node node ) throws Throwable + { + db.getNodeManager().clearCache(); + ExecutorService executor = newCachedThreadPool(); + final CountDownLatch startSignal = new CountDownLatch( 1 ); + int threads = getRuntime().availableProcessors(); + final AtomicReference error = new AtomicReference(); + for ( int i = 0; i < threads; i++ ) + { + executor.submit( new Runnable() + { + @Override + public void run() + { + awaitStartSignalAndRandomTimeLonger( startSignal ); + try + { + assertEquals( relCount, count( node.getRelationships() ) ); + } + catch ( Throwable e ) + { + error.set( e ); + } + } + } ); + } + startSignal.countDown(); + executor.shutdown(); + executor.awaitTermination( 10, SECONDS ); + + if ( error.get() != null ) + throw error.get(); + } + + private static int idleLoop( int l ) + { + int i = 0; + for ( int j = 0; j < l; j++ ) + i++; + return i; + } + + private Node createNodeWithRelationships( GraphDatabaseAPI db ) + { + Transaction tx = db.beginTx(); + Node node; + try + { + node = db.createNode(); + for ( int i = 0; i < relCount / 2; i++ ) + node.createRelationshipTo( node, MyRelTypes.TEST ); + for ( int i = 0; i < relCount / 2; i++ ) + node.createRelationshipTo( node, MyRelTypes.TEST2 ); + tx.success(); + return node; + } + finally + { + tx.finish(); + } + } +}