From e8f9159551200d9cb1ff34cdd316b44c6028ec63 Mon Sep 17 00:00:00 2001 From: Alistair Jones Date: Fri, 22 Jul 2016 14:11:20 +0100 Subject: [PATCH] Don't take locks for entities created in the same transaction. There is no need to take locks for new entities that have not yet been committed because they cannot yet be visible to any other transaction. There should have been no contention for these locks, so this change should not have a significant impact, except where the cost of acquiring any lock is high, for example when acquiring locks for a slave in an HA cluster, where each lock acquisition requires a network call. --- .../impl/api/LockingStatementOperations.java | 36 ++++-- .../api/LockingStatementOperationsTest.java | 117 ++++++++++++++++-- .../org/neo4j/kernel/impl/core/NodeTest.java | 57 --------- 3 files changed, 134 insertions(+), 76 deletions(-) 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();