Skip to content

Commit

Permalink
Lock node without relationships during detach delete
Browse files Browse the repository at this point in the history 
Update node detach delete code to properly acquire node locks in case when node does not have any relationships.
Update test cases to cover this case.
  • Loading branch information
@MishaDemianenko
MishaDemianenko committed Sep 21, 2016
1 parent 3b0bac5 commit d127cb2
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 25 deletions.
12 changes: 6 additions & 6 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java
View file
Expand Up @@ -19,8 +19,9 @@
*/
package org.neo4j.kernel.impl.api;

import org.apache.commons.lang3.mutable.MutableInt;

import java.util.Iterator;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;
import java.util.function.Predicate;

Expand Down Expand Up @@ -271,17 +272,16 @@ public void nodeDelete( KernelStatement state, long nodeId )
}

@Override
public int nodeDetachDelete( final KernelStatement state, final long nodeId )
throws EntityNotFoundException, AutoIndexingKernelException, InvalidTransactionTypeKernelException, KernelException
public int nodeDetachDelete( final KernelStatement state, final long nodeId ) throws KernelException
{
final AtomicInteger count = new AtomicInteger();
final MutableInt count = new MutableInt();
TwoPhaseNodeForRelationshipLocking locking = new TwoPhaseNodeForRelationshipLocking( entityReadDelegate,
relId -> {
state.assertOpen();
try
{
entityWriteDelegate.relationshipDelete( state, relId );
count.incrementAndGet();
count.increment();
}
catch ( EntityNotFoundException e )
{
Expand All @@ -292,7 +292,7 @@ public int nodeDetachDelete( final KernelStatement state, final long nodeId )
locking.lockAllNodesAndConsumeRelationships( nodeId, state );
state.assertOpen();
entityWriteDelegate.nodeDetachDelete( state, nodeId );
return count.get();
return count.intValue();
}

@Override
Expand Down
10 changes: 4 additions & 6 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/api/OperationsFacade.java
View file
Expand Up @@ -31,8 +31,6 @@
import org.neo4j.collection.primitive.PrimitiveLongIterator;
import org.neo4j.cursor.Cursor;
import org.neo4j.graphdb.Direction;
import org.neo4j.kernel.api.exceptions.KernelException;
import org.neo4j.kernel.api.security.AccessMode;
import org.neo4j.kernel.api.DataWriteOperations;
import org.neo4j.kernel.api.KernelTransaction;
import org.neo4j.kernel.api.LegacyIndexHits;
Expand All @@ -47,6 +45,7 @@
import org.neo4j.kernel.api.constraints.UniquenessConstraint;
import org.neo4j.kernel.api.exceptions.EntityNotFoundException;
import org.neo4j.kernel.api.exceptions.InvalidTransactionTypeKernelException;
import org.neo4j.kernel.api.exceptions.KernelException;
import org.neo4j.kernel.api.exceptions.LabelNotFoundKernelException;
import org.neo4j.kernel.api.exceptions.ProcedureException;
import org.neo4j.kernel.api.exceptions.PropertyKeyIdNotFoundKernelException;
Expand All @@ -64,17 +63,16 @@
import org.neo4j.kernel.api.exceptions.schema.IllegalTokenNameException;
import org.neo4j.kernel.api.exceptions.schema.IndexBrokenKernelException;
import org.neo4j.kernel.api.exceptions.schema.IndexSchemaRuleNotFoundException;
import org.neo4j.kernel.api.exceptions.schema.ProcedureConstraintViolation;
import org.neo4j.kernel.api.exceptions.schema.SchemaRuleNotFoundException;
import org.neo4j.kernel.api.exceptions.schema.TooManyLabelsException;
import org.neo4j.kernel.api.index.IndexDescriptor;
import org.neo4j.kernel.api.index.InternalIndexState;
import org.neo4j.kernel.api.proc.CallableProcedure;
import org.neo4j.kernel.api.proc.ProcedureSignature;
import org.neo4j.kernel.impl.proc.Procedures;
import org.neo4j.kernel.api.proc.ProcedureSignature.ProcedureName;
import org.neo4j.kernel.api.properties.DefinedProperty;
import org.neo4j.kernel.api.properties.Property;
import org.neo4j.kernel.api.security.AccessMode;
import org.neo4j.kernel.impl.api.operations.CountsOperations;
import org.neo4j.kernel.impl.api.operations.EntityReadOperations;
import org.neo4j.kernel.impl.api.operations.EntityWriteOperations;
Expand All @@ -86,6 +84,7 @@
import org.neo4j.kernel.impl.api.operations.SchemaReadOperations;
import org.neo4j.kernel.impl.api.operations.SchemaStateOperations;
import org.neo4j.kernel.impl.api.store.RelationshipIterator;
import org.neo4j.kernel.impl.proc.Procedures;
import org.neo4j.register.Register.DoubleLongRegister;
import org.neo4j.storageengine.api.NodeItem;
import org.neo4j.storageengine.api.RelationshipItem;
Expand Down Expand Up @@ -971,8 +970,7 @@ public void nodeDelete( long nodeId )
}

@Override
public int nodeDetachDelete( long nodeId ) throws EntityNotFoundException, InvalidTransactionTypeKernelException,
AutoIndexingKernelException, KernelException
public int nodeDetachDelete( long nodeId ) throws KernelException
{
statement.assertOpen();
return dataWrite().nodeDetachDelete( statement, nodeId );
Expand Down
23 changes: 11 additions & 12 deletions ...ty/kernel/src/main/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLocking.java
View file
Expand Up @@ -34,7 +34,7 @@
class TwoPhaseNodeForRelationshipLocking
{
private final PrimitiveLongSet nodeIds = Primitive.longSet();
private final EntityReadOperations reads;
private final EntityReadOperations entityReadOperations;
private final ThrowingConsumer<Long,KernelException> relIdAction;

private boolean retry = true;
Expand Down Expand Up @@ -74,45 +74,44 @@ public void visit( long relId, int type, long startNode, long endNode ) throws K
};


TwoPhaseNodeForRelationshipLocking( EntityReadOperations reads, ThrowingConsumer<Long,KernelException> relIdAction )
TwoPhaseNodeForRelationshipLocking( EntityReadOperations entityReadOperations, ThrowingConsumer<Long,KernelException> relIdAction )
{
this.reads = reads;
this.entityReadOperations = entityReadOperations;
this.relIdAction = relIdAction;
}

void lockAllNodesAndConsumeRelationships( long nodeId, final KernelStatement state ) throws KernelException
{
nodeIds.add( nodeId );
while ( retry )
{
retry = false;
first = true;
firstRelId = -1;

// lock all the nodes involved by following the node id ordering
try ( Cursor<NodeItem> cursor = reads.nodeCursorById( state, nodeId ) )
try ( Cursor<NodeItem> cursor = entityReadOperations.nodeCursorById( state, nodeId ) )
{
RelationshipIterator relationships = cursor.get().getRelationships( Direction.BOTH );
while ( relationships.hasNext() )
{
reads.relationshipVisit( state, relationships.next(), collectNodeIdVisitor );
entityReadOperations.relationshipVisit( state, relationships.next(), collectNodeIdVisitor );
}
}

PrimitiveLongIterator nodeIdIterator = nodeIds.iterator();
while ( nodeIdIterator.hasNext() )
{
PrimitiveLongIterator iterator = nodeIds.iterator();
while ( iterator.hasNext() )
{
state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, iterator.next() );
}
state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, nodeIdIterator.next() );
}

// perform the action on each relationship, we will retry if the the relationship iterator contains new relationships
try ( Cursor<NodeItem> cursor = reads.nodeCursorById( state, nodeId ) )
try ( Cursor<NodeItem> cursor = entityReadOperations.nodeCursorById( state, nodeId ) )
{
RelationshipIterator relationships = cursor.get().getRelationships( Direction.BOTH );
while ( relationships.hasNext() )
{
reads.relationshipVisit( state, relationships.next(), relationshipConsumingVisitor );
entityReadOperations.relationshipVisit( state, relationships.next(), relationshipConsumingVisitor );
if ( retry )
{
PrimitiveLongIterator iterator = nodeIds.iterator();
Expand Down
61 changes: 61 additions & 0 deletions community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java
View file
Expand Up @@ -31,6 +31,7 @@
import org.neo4j.kernel.api.constraints.UniquenessConstraint;
import org.neo4j.kernel.api.exceptions.EntityNotFoundException;
import org.neo4j.kernel.api.exceptions.InvalidTransactionTypeKernelException;
import org.neo4j.kernel.api.exceptions.KernelException;
import org.neo4j.kernel.api.exceptions.legacyindex.AutoIndexingKernelException;
import org.neo4j.kernel.api.index.IndexDescriptor;
import org.neo4j.kernel.api.properties.DefinedProperty;
Expand All @@ -43,21 +44,31 @@
import org.neo4j.kernel.impl.api.operations.SchemaReadOperations;
import org.neo4j.kernel.impl.api.operations.SchemaStateOperations;
import org.neo4j.kernel.impl.api.operations.SchemaWriteOperations;
import org.neo4j.kernel.impl.api.state.StubCursors;
import org.neo4j.kernel.impl.api.state.TxState;
import org.neo4j.kernel.impl.api.store.CursorRelationshipIterator;
import org.neo4j.kernel.impl.api.store.RelationshipIterator;
import org.neo4j.kernel.impl.api.store.StoreSingleNodeCursor;
import org.neo4j.kernel.impl.locking.Locks;
import org.neo4j.kernel.impl.locking.ResourceTypes;
import org.neo4j.kernel.impl.locking.SimpleStatementLocks;
import org.neo4j.kernel.impl.proc.Procedures;
import org.neo4j.kernel.impl.util.Cursors;
import org.neo4j.storageengine.api.Direction;
import org.neo4j.storageengine.api.NodeItem;
import org.neo4j.storageengine.api.RelationshipItem;
import org.neo4j.storageengine.api.StorageStatement;

import static org.junit.Assert.assertSame;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.when;
import static org.neo4j.kernel.impl.locking.ResourceTypes.schemaResource;

Expand Down Expand Up @@ -477,6 +488,56 @@ public void shouldNotAcquireEntityWriteLockBeforeSettingPropertyOnJustCreatedRel
order.verify( entityWriteOps ).relationshipSetProperty( state, 123, property );
}

@Test
public void detachDeleteNodeWithoutRelationshipsExclusivelyLockNode() throws KernelException
{
long nodeId = 1L;

NodeItem nodeItem = mock( NodeItem.class );
when( nodeItem.getRelationships( Direction.BOTH ) ).thenReturn( RelationshipIterator.EMPTY );
StoreSingleNodeCursor nodeCursor = mock( StoreSingleNodeCursor.class );
when( nodeCursor.get() ).thenReturn( nodeItem );
when( entityReadOps.nodeCursorById( state, nodeId ) ).thenReturn( nodeCursor );

lockingOps.nodeDetachDelete( state, nodeId );

order.verify( locks ).acquireExclusive( ResourceTypes.NODE, nodeId );
order.verify( locks, times( 0 ) ).releaseExclusive( ResourceTypes.NODE, nodeId );
order.verify( entityWriteOps ).nodeDetachDelete( state, nodeId );
}

@Test
public void detachDeleteNodeExclusivelyLockNodes() throws KernelException
{
long startNodeId = 1L;
long endNodeId = 2L;

RelationshipItem relationshipItem = StubCursors.asRelationship( 1L, 0, startNodeId, endNodeId, null );
CursorRelationshipIterator relationshipIterator =
new CursorRelationshipIterator( Cursors.cursor( relationshipItem ) );

NodeItem nodeItem = mock( NodeItem.class );
when( nodeItem.getRelationships( Direction.BOTH ) ).thenReturn( relationshipIterator );
StoreSingleNodeCursor nodeCursor = mock( StoreSingleNodeCursor.class );
when( nodeCursor.get() ).thenReturn( nodeItem );
when( entityReadOps.nodeCursorById( state, startNodeId ) ).thenReturn( nodeCursor );
doAnswer( invocation ->
{
RelationshipVisitor visitor = invocation.getArgumentAt( 2, RelationshipVisitor.class );
visitor.visit( relationshipItem.id(), relationshipItem.type(), relationshipItem.startNode(),
relationshipItem.endNode() );
return null;
} ).when( entityReadOps ).relationshipVisit( eq(state), anyLong(), any() );

lockingOps.nodeDetachDelete( state, startNodeId );

order.verify( locks ).acquireExclusive( ResourceTypes.NODE, startNodeId );
order.verify( locks ).acquireExclusive( ResourceTypes.NODE, endNodeId );
order.verify( locks, times( 0 ) ).releaseExclusive( ResourceTypes.NODE, startNodeId );
order.verify( locks, times( 0 ) ).releaseExclusive( ResourceTypes.NODE, endNodeId );
order.verify( entityWriteOps ).nodeDetachDelete( state, startNodeId );
}

private static class SimpleTxStateHolder implements TxStateHolder
{
private final TxState txState;
Expand Down
2 changes: 1 addition & 1 deletion community/kernel/src/test/java/org/neo4j/kernel/impl/api/OperationsFacadeTest.java
View file
Expand Up @@ -121,4 +121,4 @@ private TokenNameLookup getDefaultTokenNameLookup()
return tokenNameLookup;
}

}
}
15 changes: 15 additions & 0 deletions ...ernel/src/test/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLockingTest.java
View file
Expand Up @@ -48,6 +48,8 @@
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
import static org.neo4j.helpers.collection.Iterators.set;

Expand Down Expand Up @@ -120,6 +122,19 @@ public void shouldLockNodesInOrderAndConsumeTheRelationshipsAndRetryIfTheNewRela
assertEquals( set( 21L, 22L, 23L ), collector.set );
}

@Test
public void lockNodeWithoutRelationships() throws Exception
{
Collector collector = new Collector();
TwoPhaseNodeForRelationshipLocking locking = new TwoPhaseNodeForRelationshipLocking( ops, collector );
returnRelationships( nodeId, false );

locking.lockAllNodesAndConsumeRelationships( nodeId, state );

verify( locks ).acquireExclusive( ResourceTypes.NODE, nodeId );
verifyNoMoreInteractions( locks );
}

private void returnNodesForRelationship( final long relId, final long startNodeId, final long endNodeId )
throws Exception
{
Expand Down

0 comments on commit d127cb2

Please sign in to comment.