Skip to content

Commit

Permalink
Fix deadlock caused by different lock ordering on nodes for relations…
Browse files Browse the repository at this point in the history
…hips

When we create relationships we lock the nodes wrt to the node id
ordering, but when we delete a relationship we were locking the start
node first and the end node last. This was causing deadlock when
creating and deleting relationships between the same 2 ndoes.
  • Loading branch information
davidegrohmann authored and tinwelint committed Apr 15, 2016
1 parent c30f273 commit 9a19282
Show file tree
Hide file tree
Showing 3 changed files with 225 additions and 14 deletions.
Expand Up @@ -48,6 +48,9 @@
import org.neo4j.kernel.impl.locking.ResourceTypes;
import org.neo4j.kernel.impl.store.SchemaStorage;

import static java.lang.Math.max;
import static java.lang.Math.min;

import static org.neo4j.kernel.impl.locking.ResourceTypes.schemaResource;

public class LockingStatementOperations implements
Expand Down Expand Up @@ -229,17 +232,7 @@ public long nodeCreate( KernelStatement statement )
public long relationshipCreate( KernelStatement state, int relationshipTypeId, long startNodeId, long endNodeId )
throws EntityNotFoundException
{
// Order the locks to lower the risk of deadlocks with other threads adding rels concurrently
if(startNodeId < endNodeId)
{
state.locks().acquireExclusive( ResourceTypes.NODE, startNodeId );
state.locks().acquireExclusive( ResourceTypes.NODE, endNodeId );
}
else
{
state.locks().acquireExclusive( ResourceTypes.NODE, endNodeId );
state.locks().acquireExclusive( ResourceTypes.NODE, startNodeId );
}
lockRelationshipNodes( state, startNodeId, endNodeId );
return entityWriteDelegate.relationshipCreate( state, relationshipTypeId, startNodeId, endNodeId );
}

Expand All @@ -253,8 +246,7 @@ public void relationshipDelete( final KernelStatement state, long relationshipId
@Override
public void visit( long relId, int type, long startNode, long endNode )
{
state.locks().acquireExclusive( ResourceTypes.NODE, startNode );
state.locks().acquireExclusive( ResourceTypes.NODE, endNode );
lockRelationshipNodes( state, startNode, endNode );
}
});
}
Expand All @@ -266,6 +258,16 @@ public void visit( long relId, int type, long startNode, long endNode )
entityWriteDelegate.relationshipDelete( state, relationshipId );
}

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().acquireExclusive( ResourceTypes.NODE, min( startNodeId, endNodeId ) );
if ( startNodeId != endNodeId )
{
state.locks().acquireExclusive( ResourceTypes.NODE, max( startNodeId, endNodeId ) );
}
}

@Override
public UniquenessConstraint uniquenessConstraintCreate( KernelStatement state, int labelId, int propertyKeyId )
throws CreateConstraintFailureException, AlreadyConstrainedException, AlreadyIndexedException
Expand Down
Expand Up @@ -21,6 +21,8 @@

import org.junit.Test;
import org.mockito.InOrder;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;

import org.neo4j.helpers.Function;
import org.neo4j.kernel.api.constraints.UniquenessConstraint;
Expand Down Expand Up @@ -50,7 +52,8 @@ public class LockingStatementOperationsTest
private final SchemaWriteOperations schemaWriteOps;
private final Locks.Client locks = mock( Locks.Client.class );
private final InOrder order;
private final KernelStatement state = new KernelStatement( null, null, null, null, locks, null );
private final KernelTransactionImplementation transaction = mock( KernelTransactionImplementation.class );
private final KernelStatement state = new KernelStatement( transaction, null, null, null, locks, null );
private final SchemaStateOperations schemaStateOps;

public LockingStatementOperationsTest()
Expand Down Expand Up @@ -298,4 +301,98 @@ public void shouldAcquireSchemaReadLockBeforeFlushingSchemaState() throws Except
order.verify( locks ).acquireShared( ResourceTypes.SCHEMA, schemaResource() );
order.verify( schemaStateOps ).schemaStateFlush( state );
}

@Test
public void shouldAcquireNodeLocksWhenCreatingRelationshipInOrderOfAscendingId() throws Exception
{
// GIVEN
long lowId = 3;
long highId = 5;

{
// WHEN
lockingOps.relationshipCreate( state, 0, lowId, highId );

// THEN
InOrder lockingOrder = inOrder( locks );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {lowId} );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {highId} );
lockingOrder.verifyNoMoreInteractions();
reset( locks );
}

{
// WHEN
lockingOps.relationshipCreate( state, 0, highId, lowId );

// THEN
InOrder lockingOrder = inOrder( locks );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {lowId} );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {highId} );
lockingOrder.verifyNoMoreInteractions();
}
}

@SuppressWarnings( "unchecked" )
@Test
public void shouldAcquireNodeLocksWhenDeletingRelationshipInOrderOfAscendingId() throws Exception
{
// GIVEN
final long relationshipId = 10;
final long lowId = 3;
final long highId = 5;

{
// and GIVEN
doAnswer( new Answer<Void>()
{
@Override
public Void answer( InvocationOnMock invocation ) throws Throwable
{
RelationshipVisitor<RuntimeException> visitor =
(RelationshipVisitor<RuntimeException>) invocation.getArguments()[2];
visitor.visit( relationshipId, 0, lowId, highId );
return null;
}
} ).when( entityReadOps ).relationshipVisit( any( KernelStatement.class ), anyLong(),
any( RelationshipVisitor.class ) );

// WHEN
lockingOps.relationshipDelete( state, relationshipId );

// THEN
InOrder lockingOrder = inOrder( locks );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {lowId} );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {highId} );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.RELATIONSHIP, new long[] {relationshipId} );
lockingOrder.verifyNoMoreInteractions();
reset( locks );
}

{
// and GIVEN
doAnswer( new Answer<Void>()
{
@Override
public Void answer( InvocationOnMock invocation ) throws Throwable
{
RelationshipVisitor<RuntimeException> visitor =
(RelationshipVisitor<RuntimeException>) invocation.getArguments()[2];
visitor.visit( relationshipId, 0, highId, lowId );
return null;
}
} ).when( entityReadOps ).relationshipVisit( any( KernelStatement.class ), anyLong(),
any( RelationshipVisitor.class ) );

// WHEN
lockingOps.relationshipDelete( state, relationshipId );

// THEN
InOrder lockingOrder = inOrder( locks );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {lowId} );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.NODE, new long[] {highId} );
lockingOrder.verify( locks ).acquireExclusive( ResourceTypes.RELATIONSHIP, new long[] {relationshipId} );
lockingOrder.verifyNoMoreInteractions();
}
}
}
@@ -0,0 +1,112 @@
/*
* Copyright (c) 2002-2016 "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 <http://www.gnu.org/licenses/>.
*/
package org.neo4j.kernel.impl.locking;

import org.junit.Rule;
import org.junit.Test;

import org.neo4j.graphdb.Node;
import org.neo4j.graphdb.Relationship;
import org.neo4j.graphdb.Transaction;
import org.neo4j.kernel.impl.MyRelTypes;
import org.neo4j.test.DatabaseRule;
import org.neo4j.test.ImpermanentDatabaseRule;
import org.neo4j.test.Race;
import org.neo4j.test.RandomRule;
import static org.junit.Assert.assertTrue;

/**
* Testing on database level that creating and deleting relationships over the the same two nodes
* doesn't create unnecessary deadlock scenarios, i.e. that locking order is predictable and symmetrical
*/
public class RelationshipCreateDeleteLockOrderingIT
{
@Rule
public final DatabaseRule db = new ImpermanentDatabaseRule();
@Rule
public final RandomRule random = new RandomRule();

@Test
public void shouldNotDeadlockWhenConcurrentCreateAndDeleteRelationships() throws Throwable
{
// GIVEN (A) -[R]-> (B)
final Node a;
final Node b;
try ( Transaction tx = db.beginTx() )
{
(a = db.createNode()).createRelationshipTo( b = db.createNode(), MyRelTypes.TEST );
tx.success();
}

// WHEN
Race race = new Race();
// a bunch of deleters
for ( int i = 0; i < 30; i++ )
{
race.addContestant( new Runnable()
{
@Override
public void run()
{
try ( Transaction tx = db.beginTx() )
{
Node node = random.nextBoolean() ? a : b;
for ( Relationship relationship : node.getRelationships() )
{
try
{
relationship.delete();
}
catch ( IllegalStateException e )
{
// This is OK and expected since there are multiple threads deleting
assertTrue( e.getMessage().contains( "already deleted" ) );
}
}
tx.success();
}
}
} );
}

// a bunch of creators
for ( int i = 0; i < 30; i++ )
{
race.addContestant( new Runnable()
{
@Override
public void run()
{
try ( Transaction tx = db.beginTx() )
{
boolean order = random.nextBoolean();
Node start = order ? a : b;
Node end = order ? b : a;
start.createRelationshipTo( end, MyRelTypes.TEST );
tx.success();
}
}
} );
}

// THEN there should be no thread throwing exception, especially DeadlockDetectedException
race.go();
}
}

0 comments on commit 9a19282

Please sign in to comment.