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 95790e9aed6ea..aa89d6ef99881 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 @@ -109,7 +109,7 @@ public boolean nodeAddLabel( KernelStatement state, long nodeId, int labelId ) // by ConstraintEnforcingEntityOperations included the full cake, with locking included. acquireSharedSchemaLock( state ); - state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, nodeId ); + acquireExclusiveNodeLock( state, nodeId ); state.assertOpen(); return entityWriteDelegate.nodeAddLabel( state, nodeId, labelId ); @@ -118,7 +118,7 @@ public boolean nodeAddLabel( KernelStatement state, long nodeId, int labelId ) @Override public boolean nodeRemoveLabel( KernelStatement state, long nodeId, int labelId ) throws EntityNotFoundException { - state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, nodeId ); + acquireExclusiveNodeLock( state, nodeId ); state.assertOpen(); return entityWriteDelegate.nodeRemoveLabel( state, nodeId, labelId ); } @@ -258,7 +258,7 @@ public Iterator uniqueIndexesGetAll( KernelStatement state ) @Override public void nodeDelete( KernelStatement state, long nodeId ) throws EntityNotFoundException { - state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, nodeId ); + acquireExclusiveNodeLock( state, nodeId ); state.assertOpen(); entityWriteDelegate.nodeDelete( state, nodeId ); } @@ -319,7 +319,7 @@ public void visit(long relId, int type, long startNode, long endNode) { lockRelationshipNodes(state, startNode, endNode); } }); - state.locks().optimistic().acquireExclusive(ResourceTypes.RELATIONSHIP, relationshipId); + acquireExclusiveRelationshipLock( state, relationshipId ); state.assertOpen(); entityWriteDelegate.relationshipDelete(state, relationshipId); } @@ -327,10 +327,10 @@ public void visit(long relId, int type, long startNode, long endNode) { private void lockRelationshipNodes( KernelStatement state, long startNodeId, long endNodeId ) { // Order the locks to lower the risk of deadlocks with other threads creating/deleting rels concurrently - state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, min( startNodeId, endNodeId ) ); + acquireExclusiveNodeLock( state, min( startNodeId, endNodeId ) ); if ( startNodeId != endNodeId ) { - state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, max( startNodeId, endNodeId ) ); + acquireExclusiveNodeLock( state, max( startNodeId, endNodeId ) ); } } @@ -473,7 +473,7 @@ public Property nodeSetProperty( KernelStatement state, long nodeId, DefinedProp // by ConstraintEnforcingEntityOperations included the full cake, with locking included. acquireSharedSchemaLock( state ); - state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, nodeId ); + acquireExclusiveNodeLock( state, nodeId ); state.assertOpen(); return entityWriteDelegate.nodeSetProperty( state, nodeId, property ); } @@ -482,7 +482,7 @@ public Property nodeSetProperty( KernelStatement state, long nodeId, DefinedProp public Property nodeRemoveProperty( KernelStatement state, long nodeId, int propertyKeyId ) throws EntityNotFoundException { - state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, nodeId ); + acquireExclusiveNodeLock( state, nodeId ); state.assertOpen(); return entityWriteDelegate.nodeRemoveProperty( state, nodeId, propertyKeyId ); } @@ -492,7 +492,7 @@ public Property relationshipSetProperty( KernelStatement state, long relationshipId, DefinedProperty property ) throws EntityNotFoundException { - state.locks().optimistic().acquireExclusive( ResourceTypes.RELATIONSHIP, relationshipId ); + acquireExclusiveRelationshipLock( state, relationshipId ); state.assertOpen(); return entityWriteDelegate.relationshipSetProperty( state, relationshipId, property ); } @@ -502,7 +502,7 @@ public Property relationshipRemoveProperty( KernelStatement state, long relationshipId, int propertyKeyId ) throws EntityNotFoundException { - state.locks().optimistic().acquireExclusive( ResourceTypes.RELATIONSHIP, relationshipId ); + acquireExclusiveRelationshipLock( state, relationshipId ); state.assertOpen(); return entityWriteDelegate.relationshipRemoveProperty( state, relationshipId, propertyKeyId ); } @@ -559,6 +559,22 @@ public String indexGetFailure( Statement state, IndexDescriptor descriptor ) return schemaReadDelegate.indexGetFailure( state, descriptor ); } + private void acquireExclusiveNodeLock( KernelStatement state, long nodeId ) + { + if ( !state.txState().nodeIsAddedInThisTx( nodeId ) ) + { + state.locks().optimistic().acquireExclusive( ResourceTypes.NODE, nodeId ); + } + } + + private void acquireExclusiveRelationshipLock( KernelStatement state, long relationshipId ) + { + if ( !state.txState().relationshipIsAddedInThisTx( relationshipId ) ) + { + state.locks().optimistic().acquireExclusive( ResourceTypes.RELATIONSHIP, relationshipId ); + } + } + private void acquireSharedSchemaLock( KernelStatement state ) { state.locks().optimistic().acquireShared( ResourceTypes.SCHEMA, schemaResource() ); 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 516eecbe79bd4..20276be01fff1 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 @@ -35,11 +35,15 @@ import org.neo4j.kernel.api.index.IndexDescriptor; import org.neo4j.kernel.api.properties.DefinedProperty; import org.neo4j.kernel.api.properties.Property; +import org.neo4j.kernel.api.txstate.LegacyIndexTransactionState; +import org.neo4j.kernel.api.txstate.TransactionState; +import org.neo4j.kernel.api.txstate.TxStateHolder; import org.neo4j.kernel.impl.api.operations.EntityReadOperations; import org.neo4j.kernel.impl.api.operations.EntityWriteOperations; 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.TxState; import org.neo4j.kernel.impl.locking.Locks; import org.neo4j.kernel.impl.locking.ResourceTypes; import org.neo4j.kernel.impl.locking.SimpleStatementLocks; @@ -50,6 +54,7 @@ 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.when; import static org.neo4j.function.Functions.constant; @@ -65,8 +70,9 @@ public class LockingStatementOperationsTest private final Locks.Client locks = mock( Locks.Client.class ); private final InOrder order; private final KernelTransactionImplementation transaction = mock( KernelTransactionImplementation.class ); - private final KernelStatement state = new KernelStatement( transaction, null, null, null, - new SimpleStatementLocks( locks ), null, null ); + private final TxState txState = new TxState(); + private final KernelStatement state = new KernelStatement( transaction, null, null, + new SimpleTxStateHolder( txState ), new SimpleStatementLocks( locks ), null, null ); private final SchemaStateOperations schemaStateOps; public LockingStatementOperationsTest() @@ -83,25 +89,25 @@ public LockingStatementOperationsTest() } @Test - public void shouldAcquireEntityWriteLockCreatingRelationship() throws Exception + public void shouldAcquireEntityWriteLockBeforeAddingLabelToNode() throws Exception { // when - lockingOps.relationshipCreate( state, 1, 2, 3 ); + lockingOps.nodeAddLabel( state, 123, 456 ); // then - order.verify( locks ).acquireExclusive( ResourceTypes.NODE, 2 ); - order.verify( locks ).acquireExclusive( ResourceTypes.NODE, 3 ); - order.verify( entityWriteOps ).relationshipCreate( state, 1, 2, 3 ); + order.verify( locks ).acquireExclusive( ResourceTypes.NODE, 123 ); + order.verify( entityWriteOps ).nodeAddLabel( state, 123, 456 ); } @Test - public void shouldAcquireEntityWriteLockBeforeAddingLabelToNode() throws Exception + public void shouldNotAcquireEntityWriteLockBeforeAddingLabelToJustCreatedNode() throws Exception { // when + txState.nodeDoCreate( 123 ); lockingOps.nodeAddLabel( state, 123, 456 ); // then - order.verify( locks ).acquireExclusive( ResourceTypes.NODE, 123 ); + order.verify( locks, never() ).acquireExclusive( ResourceTypes.NODE, 123 ); order.verify( entityWriteOps ).nodeAddLabel( state, 123, 456 ); } @@ -130,6 +136,21 @@ public void shouldAcquireEntityWriteLockBeforeSettingPropertyOnNode() throws Exc order.verify( entityWriteOps ).nodeSetProperty( state, 123, property ); } + @Test + public void shouldNotAcquireEntityWriteLockBeforeSettingPropertyOnJustCreatedNode() throws Exception + { + // given + txState.nodeDoCreate( 123 ); + DefinedProperty property = Property.property( 8, 9 ); + + // when + lockingOps.nodeSetProperty( state, 123, property ); + + // then + order.verify( locks, never() ).acquireExclusive( ResourceTypes.NODE, 123 ); + order.verify( entityWriteOps ).nodeSetProperty( state, 123, property ); + } + @Test public void shouldAcquireSchemaReadLockBeforeSettingPropertyOnNode() throws Exception { @@ -155,6 +176,18 @@ public void shouldAcquireEntityWriteLockBeforeDeletingNode() throws EntityNotFou order.verify( entityWriteOps ).nodeDelete( state, 123 ); } + @Test + public void shouldNotAcquireEntityWriteLockBeforeDeletingJustCreatedNode() throws EntityNotFoundException + { + // WHEN + txState.nodeDoCreate( 123 ); + lockingOps.nodeDelete( state, 123 ); + + //THEN + order.verify( locks, never() ).acquireExclusive( ResourceTypes.NODE, 123 ); + order.verify( entityWriteOps ).nodeDelete( state, 123 ); + } + @Test public void shouldAcquireSchemaWriteLockBeforeAddingIndexRule() throws Exception { @@ -315,6 +348,18 @@ public void shouldAcquireSchemaReadLockBeforeFlushingSchemaState() throws Except order.verify( schemaStateOps ).schemaStateFlush( state ); } + @Test + public void shouldAcquireEntityWriteLockCreatingRelationship() throws Exception + { + // when + lockingOps.relationshipCreate( state, 1, 2, 3 ); + + // then + order.verify( locks ).acquireExclusive( ResourceTypes.NODE, 2 ); + order.verify( locks ).acquireExclusive( ResourceTypes.NODE, 3 ); + order.verify( entityWriteOps ).relationshipCreate( state, 1, 2, 3 ); + } + @Test public void shouldAcquireNodeLocksWhenCreatingRelationshipInOrderOfAscendingId() throws Exception { @@ -408,4 +453,58 @@ public Void answer( InvocationOnMock invocation ) throws Throwable lockingOrder.verifyNoMoreInteractions(); } } + + @Test + public void shouldAcquireEntityWriteLockBeforeSettingPropertyOnRelationship() throws Exception + { + // given + DefinedProperty property = Property.property( 8, 9 ); + + // when + lockingOps.relationshipSetProperty( state, 123, property ); + + // then + order.verify( locks ).acquireExclusive( ResourceTypes.RELATIONSHIP, 123 ); + order.verify( entityWriteOps ).relationshipSetProperty( state, 123, property ); + } + + @Test + public void shouldNotAcquireEntityWriteLockBeforeSettingPropertyOnJustCreatedRelationship() throws Exception + { + // given + txState.relationshipDoCreate( 123, 1, 2, 3 ); + DefinedProperty property = Property.property( 8, 9 ); + + // when + lockingOps.relationshipSetProperty( state, 123, property ); + + // then + order.verify( locks, never() ).acquireExclusive( ResourceTypes.RELATIONSHIP, 123 ); + order.verify( entityWriteOps ).relationshipSetProperty( state, 123, property ); + } + + private static class SimpleTxStateHolder implements TxStateHolder + { + private final TxState txState; + + private SimpleTxStateHolder( TxState txState ) + { + this.txState = txState; + } + + @Override public TransactionState txState() + { + return txState; + } + + @Override public LegacyIndexTransactionState legacyIndexTxState() + { + return null; + } + + @Override public boolean hasTxStateWithChanges() + { + return false; + } + } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/core/NodeTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/core/NodeTest.java index 5c42a3c4658c8..b2758baf81723 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/core/NodeTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/core/NodeTest.java @@ -45,8 +45,6 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import static org.neo4j.helpers.Exceptions.launderedException; - public class NodeTest { @ClassRule @@ -426,61 +424,6 @@ public void testChangeProperty2() assertEquals( "test4", node.getProperty( "test" ) ); } - @Test - public void testNodeLockingProblem() throws InterruptedException - { - testLockProblem( getGraphDb().createNode() ); - } - - @Test - public void testRelationshipLockingProblem() throws InterruptedException - { - Node node = getGraphDb().createNode(); - Node node2 = getGraphDb().createNode(); - testLockProblem( node.createRelationshipTo( node2, - DynamicRelationshipType.withName( "lock-rel" ) ) ); - } - - private void testLockProblem( final PropertyContainer entity ) throws InterruptedException - { - entity.setProperty( "key", "value" ); - final AtomicBoolean gotTheLock = new AtomicBoolean(); - Thread thread = new Thread() - { - @Override - public void run() - { - try( Transaction tx = getGraphDb().beginTx() ) - { - tx.acquireWriteLock( entity ); - gotTheLock.set( true ); - tx.success(); - } - catch ( Exception e ) - { - e.printStackTrace(); - throw launderedException( e ); - } - } - }; - thread.start(); - long endTime = System.currentTimeMillis() + 5000; - while ( thread.getState() != State.TERMINATED ) - { - if ( thread.getState() == Thread.State.WAITING || thread.getState() == State.TIMED_WAITING) - { - break; - } - Thread.sleep( 1 ); - if ( System.currentTimeMillis() > endTime ) break; - } - boolean gotLock = gotTheLock.get(); - tx.success(); - tx.begin(); - assertFalse( gotLock ); - thread.join(); - } - private GraphDatabaseService getGraphDb() { return db.getGraphDatabaseService();