diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java index 8a659cf43f094..c05e6122cd5d2 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java @@ -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; @@ -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 ) { @@ -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 diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/OperationsFacade.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/OperationsFacade.java index 206987555c8e2..c788acb59c36c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/OperationsFacade.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/OperationsFacade.java @@ -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; @@ -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; @@ -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; @@ -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; @@ -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 ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLocking.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLocking.java index e8a95ff4760c9..56e7b24fcbe72 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLocking.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLocking.java @@ -34,7 +34,7 @@ class TwoPhaseNodeForRelationshipLocking { private final PrimitiveLongSet nodeIds = Primitive.longSet(); - private final EntityReadOperations reads; + private final EntityReadOperations entityReadOperations; private final ThrowingConsumer relIdAction; private boolean retry = true; @@ -74,14 +74,15 @@ public void visit( long relId, int type, long startNode, long endNode ) throws K }; - TwoPhaseNodeForRelationshipLocking( EntityReadOperations reads, ThrowingConsumer relIdAction ) + TwoPhaseNodeForRelationshipLocking( EntityReadOperations entityReadOperations, ThrowingConsumer 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; @@ -89,30 +90,28 @@ void lockAllNodesAndConsumeRelationships( long nodeId, final KernelStatement sta firstRelId = -1; // lock all the nodes involved by following the node id ordering - try ( Cursor cursor = reads.nodeCursorById( state, nodeId ) ) + try ( Cursor 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 cursor = reads.nodeCursorById( state, nodeId ) ) + try ( Cursor 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(); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java index efc5dfd62e7f0..14e01b864ecd9 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java @@ -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; @@ -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; @@ -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; diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/OperationsFacadeTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/OperationsFacadeTest.java index 57dacf7c20801..a742619b58870 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/OperationsFacadeTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/OperationsFacadeTest.java @@ -121,4 +121,4 @@ private TokenNameLookup getDefaultTokenNameLookup() return tokenNameLookup; } -} \ No newline at end of file +} diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLockingTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLockingTest.java index 8557c33f7270f..c2aacc61d4e15 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLockingTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/TwoPhaseNodeForRelationshipLockingTest.java @@ -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; @@ -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 {