From 1eb32a0d2060f888a767f3a338d1bd5f3df26e57 Mon Sep 17 00:00:00 2001 From: Mattias Persson Date: Mon, 7 Dec 2015 09:21:56 +0100 Subject: [PATCH] KernelTransactionImplementation doesn't reference Locks.Client since those instances are hard wired to a specific Locks instance. Other similar references are always kept on the service level, like CommitProcess, where there's a proxy in between which switches when role switches. This commit undos parts of #5817 because of the issue described in #5996 and because this solution where Locks is referenced conforms with other types of references and is a simpler fix to the original problem. --- .../api/KernelTransactionImplementation.java | 34 +++++----- .../kernel/impl/api/KernelTransactions.java | 13 ++-- .../kernel/api/KernelTransactionFactory.java | 12 ++-- .../KernelTransactionImplementationTest.java | 62 +++++++++++++++++-- .../neo4j/kernel/impl/locking/NoOpLocks.java | 48 ++++++++++++++ ...ransactionThroughMasterSwitchStressIT.java | 6 +- 6 files changed, 134 insertions(+), 41 deletions(-) create mode 100644 community/kernel/src/test/java/org/neo4j/kernel/impl/locking/NoOpLocks.java diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactionImplementation.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactionImplementation.java index 1fd0381284329..c0df8da715dfb 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactionImplementation.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactionImplementation.java @@ -63,16 +63,16 @@ import org.neo4j.kernel.impl.transaction.TransactionMonitor; import org.neo4j.kernel.impl.transaction.command.Command; import org.neo4j.kernel.impl.transaction.log.PhysicalTransactionRepresentation; +import org.neo4j.kernel.impl.transaction.state.NeoStoreTransactionContext; import org.neo4j.kernel.impl.transaction.state.TransactionRecordState; import org.neo4j.kernel.impl.transaction.tracing.CommitEvent; import org.neo4j.kernel.impl.transaction.tracing.TransactionEvent; import org.neo4j.kernel.impl.transaction.tracing.TransactionTracer; import org.neo4j.kernel.impl.util.collection.ArrayCollection; -import static org.neo4j.kernel.impl.api.TransactionApplicationMode.INTERNAL; - import static org.neo4j.kernel.api.ReadOperations.ANY_LABEL; import static org.neo4j.kernel.api.ReadOperations.ANY_RELATIONSHIP_TYPE; +import static org.neo4j.kernel.impl.api.TransactionApplicationMode.INTERNAL; /** * This class should replace the {@link org.neo4j.kernel.api.KernelTransaction} interface, and take its name, as soon @@ -144,6 +144,7 @@ TransactionType upgradeToSchemaTransaction() throws InvalidTransactionTypeKernel private final Clock clock; private final TransactionToRecordStateVisitor txStateToRecordStateVisitor = new TransactionToRecordStateVisitor(); private final Collection extractedCommands = new ArrayCollection<>( 32 ); + private final Locks locksManager; private TransactionState txState; private LegacyIndexTransactionState legacyIndexTransactionState; private TransactionType transactionType = TransactionType.ANY; @@ -162,6 +163,7 @@ TransactionType upgradeToSchemaTransaction() throws InvalidTransactionTypeKernel private final TransactionTracer tracer; private TransactionEvent transactionEvent; private CloseListener closeListener; + private final NeoStoreTransactionContext context; public KernelTransactionImplementation( StatementOperationParts operations, SchemaWriteGuard schemaWriteGuard, LabelScanStore labelScanStore, @@ -170,7 +172,7 @@ public KernelTransactionImplementation( StatementOperationParts operations, TransactionRecordState recordState, RecordStateForCacheAccessor recordStateForCache, SchemaIndexProviderMap providerMap, NeoStore neoStore, - Locks.Client locks, TransactionHooks hooks, + Locks locks, TransactionHooks hooks, ConstraintIndexCreator constraintIndexCreator, TransactionHeaderInformationFactory headerInformationFactory, TransactionCommitProcess commitProcess, @@ -180,7 +182,8 @@ public KernelTransactionImplementation( StatementOperationParts operations, LegacyIndexTransactionState legacyIndexTransactionState, Pool pool, Clock clock, - TransactionTracer tracer ) + TransactionTracer tracer, + NeoStoreTransactionContext context ) { this.operations = operations; this.schemaWriteGuard = schemaWriteGuard; @@ -190,14 +193,15 @@ public KernelTransactionImplementation( StatementOperationParts operations, this.recordStateForCache = recordStateForCache; this.providerMap = providerMap; this.schemaState = schemaState; + this.locksManager = locks; this.hooks = hooks; - this.locks = locks; this.constraintIndexCreator = constraintIndexCreator; this.headerInformationFactory = headerInformationFactory; this.commitProcess = commitProcess; this.transactionMonitor = transactionMonitor; this.persistenceCache = persistenceCache; this.storeLayer = storeLayer; + this.context = context; this.legacyIndexTransactionState = new CachingLegacyIndexTransactionState( legacyIndexTransactionState ); this.pool = pool; this.clock = clock; @@ -208,8 +212,9 @@ public KernelTransactionImplementation( StatementOperationParts operations, /** Reset this transaction to a vanilla state, turning it into a logically new transaction. */ public KernelTransactionImplementation initialize( long lastCommittedTx ) { - assert locks != null : "This transaction has been disposed off, it should not be used."; - this.closing = closed = failure = success = false; + this.locks = locksManager.newClient(); + this.context.bind( locks ); + this.closing = closed = failure = success = terminated = false; this.transactionType = TransactionType.ANY; this.beforeHookInvoked = false; this.recordState.initialize( lastCommittedTx ); @@ -622,18 +627,9 @@ private void afterRollback() /** Release resources held up by this transaction & return it to the transaction pool. */ private void release() { - locks.releaseAll(); - if ( terminated ) - { - // This transaction has been externally marked for termination. - // Just dispose of this transaction and don't return it to the pool. - dispose(); - } - else - { - // Return this instance to the pool so that another transaction may use it. - pool.release( this ); - } + locks.close(); + locks = null; + pool.release( this ); } private class TransactionToRecordStateVisitor extends TxStateVisitor.Adapter diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java index f35cbab2b93e7..d3df5cae1dcf0 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java @@ -150,8 +150,6 @@ public KernelTransactions( NeoStoreTransactionContextSupplier neoStoreTransactio public KernelTransactionImplementation newInstance() { NeoStoreTransactionContext context = neoStoreTransactionContextSupplier.acquire(); - Locks.Client locksClient = locks.newClient(); - context.bind( locksClient ); TransactionRecordState recordState = new TransactionRecordState( neoStore, integrityValidator, context ); LegacyIndexTransactionState legacyIndexTransactionState = @@ -161,9 +159,10 @@ public KernelTransactionImplementation newInstance() KernelTransactionImplementation tx = new KernelTransactionImplementation( statementOperations, schemaWriteGuard, labelScanStore, indexingService, updateableSchemaState, recordState, recordStateForCache, providerMap, - neoStore, locksClient, hooks, constraintIndexCreator, transactionHeaderInformationFactory, + neoStore, locks, hooks, constraintIndexCreator, transactionHeaderInformationFactory, transactionCommitProcess, transactionMonitor, persistenceCache, storeLayer, - legacyIndexTransactionState, localTxPool, Clock.SYSTEM_CLOCK, tracers.transactionTracer ); + legacyIndexTransactionState, localTxPool, Clock.SYSTEM_CLOCK, tracers.transactionTracer, + context ); allTransactions.add( tx ); @@ -224,9 +223,7 @@ public void disposeAll() { for ( KernelTransactionImplementation tx : allTransactions ) { - // we mark all transactions for termination since we want to make sure these transactions - // won't be reused, ever. Each transaction has, among other things, a Locks.Client and we - // certainly want to keep that from being reused from this point. + // We mark all transactions for termination since we want to be on the safe side here. tx.markForTermination(); } localTxPool.disposeAll(); @@ -250,6 +247,4 @@ private void assertDatabaseIsRunning() throw new DatabaseShutdownException(); } } - - } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionFactory.java b/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionFactory.java index 65135f2cc851f..a72f3e1456521 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionFactory.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionFactory.java @@ -32,12 +32,14 @@ import org.neo4j.kernel.impl.api.state.ConstraintIndexCreator; import org.neo4j.kernel.impl.api.store.PersistenceCache; import org.neo4j.kernel.impl.api.store.StoreReadLayer; -import org.neo4j.kernel.impl.locking.NoOpClient; +import org.neo4j.kernel.impl.locking.Locks; +import org.neo4j.kernel.impl.locking.NoOpLocks; import org.neo4j.kernel.impl.store.NeoStore; -import org.neo4j.kernel.impl.transaction.tracing.TransactionTracer; import org.neo4j.kernel.impl.transaction.TransactionHeaderInformationFactory; import org.neo4j.kernel.impl.transaction.TransactionMonitor; +import org.neo4j.kernel.impl.transaction.state.NeoStoreTransactionContext; import org.neo4j.kernel.impl.transaction.state.TransactionRecordState; +import org.neo4j.kernel.impl.transaction.tracing.TransactionTracer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -54,7 +56,8 @@ static KernelTransaction kernelTransaction() mock( SchemaWriteGuard.class ), null, null, null, mock( TransactionRecordState.class ), mock( RecordStateForCacheAccessor.class ), - null, mock( NeoStore.class ), new NoOpClient(), new TransactionHooks(), + null, mock( NeoStore.class ), new NoOpLocks(), + new TransactionHooks(), mock( ConstraintIndexCreator.class ), headerInformationFactory, mock( TransactionRepresentationCommitProcess.class ), mock( TransactionMonitor.class ), mock( PersistenceCache.class ), @@ -62,6 +65,7 @@ null, mock( NeoStore.class ), new NoOpClient(), new TransactionHooks(), mock( LegacyIndexTransactionState.class ), mock(Pool.class), Clock.SYSTEM_CLOCK, - TransactionTracer.NULL ); + TransactionTracer.NULL, + mock( NeoStoreTransactionContext.class ) ); } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionImplementationTest.java b/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionImplementationTest.java index 8b5b9b4dc8f2a..22c28574e7dff 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionImplementationTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/api/KernelTransactionImplementationTest.java @@ -36,13 +36,19 @@ import org.neo4j.kernel.impl.api.TransactionCommitProcess; import org.neo4j.kernel.impl.api.TransactionHeaderInformation; import org.neo4j.kernel.impl.api.TransactionHooks; +import org.neo4j.kernel.impl.api.store.PersistenceCache; +import org.neo4j.kernel.impl.api.store.StoreReadLayer; import org.neo4j.kernel.impl.locking.LockGroup; +import org.neo4j.kernel.impl.locking.Locks; import org.neo4j.kernel.impl.locking.NoOpClient; +import org.neo4j.kernel.impl.locking.NoOpLocks; +import org.neo4j.kernel.impl.locking.community.CommunityLockManger; import org.neo4j.kernel.impl.store.NeoStore; import org.neo4j.kernel.impl.transaction.TransactionHeaderInformationFactory; import org.neo4j.kernel.impl.transaction.TransactionMonitor; import org.neo4j.kernel.impl.transaction.TransactionRepresentation; import org.neo4j.kernel.impl.transaction.command.Command; +import org.neo4j.kernel.impl.transaction.state.NeoStoreTransactionContext; import org.neo4j.kernel.impl.transaction.state.TransactionRecordState; import org.neo4j.kernel.impl.transaction.tracing.CommitEvent; import org.neo4j.kernel.impl.transaction.tracing.TransactionTracer; @@ -50,9 +56,13 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyListOf; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -356,7 +366,7 @@ public Void answer( InvocationOnMock invocationOnMock ) throws Throwable } @Test - public void shouldNotReturnTransactionInstanceWithTerminationMarkToPool() throws Exception + public void shouldStillReturnTransactionInstanceWithTerminationMarkToPool() throws Exception { // GIVEN KernelTransactionImplementation transaction = newTransaction(); @@ -366,7 +376,43 @@ public void shouldNotReturnTransactionInstanceWithTerminationMarkToPool() throws transaction.close(); // THEN - verifyZeroInteractions( pool ); + verify( pool ).release( transaction ); + } + + @Test + public void shouldBeAbleToReuseTerminatedTransaction() throws Exception + { + // GIVEN + KernelTransactionImplementation transaction = newTransaction(); + transaction.close(); + transaction.markForTermination(); + + // WHEN + transaction.initialize( 10L ); + transaction.txState().nodeDoCreate( 11L ); + transaction.success(); + transaction.close(); + + // THEN + verify( commitProcess ).commit( any( TransactionRepresentation.class ), any( LockGroup.class ), + any( CommitEvent.class ), any( TransactionApplicationMode.class ) ); + } + + @Test + public void shouldAcquireNewLocksClientEveryTimeTransactionIsReused() throws Exception + { + // GIVEN + KernelTransactionImplementation transaction = newTransaction(); + transaction.close(); + verify( locks ).newClient(); + reset( locks ); + + // WHEN + transaction.initialize( 10L ); + transaction.close(); + + // THEN + verify( locks ).newClient(); } private final NeoStore neoStore = mock( NeoStore.class ); @@ -375,12 +421,15 @@ public void shouldNotReturnTransactionInstanceWithTerminationMarkToPool() throws private final RecordStateForCacheAccessor recordStateAccessor = mock( RecordStateForCacheAccessor.class ); private final LegacyIndexTransactionState legacyIndexState = mock( LegacyIndexTransactionState.class ); private final TransactionMonitor transactionMonitor = mock( TransactionMonitor.class ); - private final CapturingCommitProcess commitProcess = new CapturingCommitProcess(); + private final CapturingCommitProcess commitProcess = spy( new CapturingCommitProcess() ); private final TransactionHeaderInformation headerInformation = mock( TransactionHeaderInformation.class ); private final TransactionHeaderInformationFactory headerInformationFactory = mock( TransactionHeaderInformationFactory.class ); private final FakeClock clock = new FakeClock(); private final Pool pool = mock( Pool.class ); + private final Locks locks = spy( new NoOpLocks() ); + private final PersistenceCache persistenceCache = mock( PersistenceCache.class ); + private final StoreReadLayer storeReadLayer = mock( StoreReadLayer.class ); @Before public void before() @@ -392,9 +441,10 @@ public void before() private KernelTransactionImplementation newTransaction() { KernelTransactionImplementation transaction = new KernelTransactionImplementation( - null, null, null, null, null, recordState, recordStateAccessor, null, neoStore, new NoOpClient(), - hooks, null, headerInformationFactory, commitProcess, transactionMonitor, null, null, - legacyIndexState, pool, clock, TransactionTracer.NULL ); + null, null, null, null, null, recordState, recordStateAccessor, null, neoStore, + locks, hooks, null, headerInformationFactory, commitProcess, transactionMonitor, + persistenceCache, storeReadLayer, + legacyIndexState, pool, clock, TransactionTracer.NULL, mock( NeoStoreTransactionContext.class ) ); transaction.initialize( 0 ); return transaction; } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/NoOpLocks.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/NoOpLocks.java new file mode 100644 index 0000000000000..bc9f1d0213e5a --- /dev/null +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/NoOpLocks.java @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2002-2015 "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 . + */ +package org.neo4j.kernel.impl.locking; + +import org.neo4j.kernel.lifecycle.LifecycleAdapter; + +public class NoOpLocks extends LifecycleAdapter implements Locks +{ + private boolean closed; + + @Override + public void shutdown() throws Throwable + { + closed = true; + } + + @Override + public Client newClient() + { + if ( closed ) + { + throw new IllegalStateException(); + } + return new NoOpClient(); + } + + @Override + public void accept( Visitor visitor ) + { + } +} diff --git a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/transaction/TransactionThroughMasterSwitchStressIT.java b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/transaction/TransactionThroughMasterSwitchStressIT.java index 6d5b086cda570..11c3a80c5e7b2 100644 --- a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/transaction/TransactionThroughMasterSwitchStressIT.java +++ b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/transaction/TransactionThroughMasterSwitchStressIT.java @@ -84,19 +84,19 @@ public void shouldNotHaveTransactionsRunningThroughRoleSwitchProduceInconsistenc { // Duration of this test. If the timeout is hit in the middle of a round, the round will be completed // and exit after that. + ManagedCluster cluster = clusterRule.startCluster(); long duration = parseTimeMillis.apply( System.getProperty( getClass().getName() + ".duration", "30s" ) ); long endTime = currentTimeMillis() + duration; while ( currentTimeMillis() < endTime ) { - oneRound(); + oneRound( cluster ); } } - private void oneRound() throws Throwable + private void oneRound( ManagedCluster cluster ) throws Throwable { // GIVEN a cluster and a node final String key = "key"; - ManagedCluster cluster = clusterRule.startCluster(); final GraphDatabaseService master = cluster.getMaster(); final long nodeId = createNode( master ); cluster.sync();