Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
core-edge: fix deadlock between shared discovery service and listener
Previously the RaftDiscoveryServiceConnector would access the discovery service when invoked on its topology change callback, but within a synchronized block while at the same time accessing the services of the discovery service which might be busy trying to invoke the callback for some other trigger, leading to a deadlock. This is now fixed by the discovery client never explcitly asking about the topology, but rather only being notified when it changes and keeping that value. Also fixes a non-synchronized iteration over the listeners in the SharedDiscoveryCoreClient.
- Loading branch information
1 parent
1f22cd0
commit b1e96d6
Showing
7 changed files
with
209 additions
and
36 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
128 changes: 128 additions & 0 deletions
128
...rprise/core-edge/src/test/java/org/neo4j/coreedge/discovery/SharedDiscoveryServiceIT.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /* | ||
| * 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 Affero 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 Affero General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Affero General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
| package org.neo4j.coreedge.discovery; | ||
|
|
||
| import org.junit.Test; | ||
| import org.mockito.ArgumentCaptor; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.UUID; | ||
| import java.util.concurrent.Callable; | ||
| import java.util.concurrent.ExecutionException; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.Future; | ||
|
|
||
| import org.neo4j.coreedge.core.CoreEdgeClusterSettings; | ||
| import org.neo4j.coreedge.core.consensus.RaftMachine; | ||
| import org.neo4j.coreedge.identity.MemberId; | ||
| import org.neo4j.graphdb.factory.GraphDatabaseSettings; | ||
| import org.neo4j.kernel.configuration.Config; | ||
| import org.neo4j.logging.NullLogProvider; | ||
|
|
||
| import static java.util.concurrent.TimeUnit.MILLISECONDS; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.junit.Assert.fail; | ||
| import static org.mockito.Mockito.atLeastOnce; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.neo4j.helpers.collection.MapUtil.stringMap; | ||
| import static org.neo4j.test.assertion.Assert.assertEventually; | ||
|
|
||
| public class SharedDiscoveryServiceIT | ||
| { | ||
| private static final long TIMEOUT_MS = 15_000; | ||
| private static final long RUN_TIME_MS = 1000;; | ||
|
|
||
| private NullLogProvider logProvider = NullLogProvider.getInstance(); | ||
|
|
||
| @Test(timeout = TIMEOUT_MS) | ||
| public void shouldDiscoverCompleteTargetSetWithoutDeadlocks() throws Exception | ||
| { | ||
| // given | ||
| ExecutorService es = Executors.newCachedThreadPool(); | ||
|
|
||
| long endTimeMillis = System.currentTimeMillis() + RUN_TIME_MS; | ||
| while ( endTimeMillis > System.currentTimeMillis() ) | ||
| { | ||
| Set<MemberId> members = new HashSet<>(); | ||
| for ( int i = 0; i < 3; i++ ) | ||
| { | ||
| members.add( new MemberId( UUID.randomUUID() ) ); | ||
| } | ||
|
|
||
| SharedDiscoveryService sharedService = new SharedDiscoveryService(); | ||
|
|
||
| List<Callable<Void>> discoveryJobs = new ArrayList<>(); | ||
| for ( MemberId member : members ) | ||
| { | ||
| discoveryJobs.add( createDiscoveryJob( member, sharedService, members ) ); | ||
| } | ||
|
|
||
| List<Future<Void>> results = es.invokeAll( discoveryJobs ); | ||
| for ( Future<Void> result : results ) | ||
| { | ||
| result.get( TIMEOUT_MS, MILLISECONDS ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private Callable<Void> createDiscoveryJob( MemberId member, DiscoveryServiceFactory disoveryServiceFactory, Set<MemberId> expectedTargetSet ) throws ExecutionException, InterruptedException | ||
| { | ||
| CoreTopologyService topologyService = disoveryServiceFactory.coreTopologyService( config(), member, mock( DiscoveredMemberRepository.class ), logProvider ); | ||
| return sharedClientStarter( topologyService, expectedTargetSet ); | ||
| } | ||
|
|
||
| private Config config() | ||
| { | ||
| return new Config( stringMap( | ||
| CoreEdgeClusterSettings.raft_advertised_address.name(), "127.0.0.1:7000", | ||
| CoreEdgeClusterSettings.transaction_advertised_address.name(), "127.0.0.1:7001", | ||
| GraphDatabaseSettings.bolt_advertised_address.name(), "127.0.0.1:7002" ) ); | ||
| } | ||
|
|
||
| private Callable<Void> sharedClientStarter( CoreTopologyService topologyService, Set<MemberId> expectedTargetSet ) | ||
| { | ||
| return () -> | ||
| { | ||
| try | ||
| { | ||
| RaftMachine raftMock = mock( RaftMachine.class ); | ||
| topologyService.start(); | ||
| topologyService.addCoreTopologyListener( new RaftDiscoveryServiceConnector( topologyService, raftMock ) ); | ||
|
|
||
| assertEventually( "should discover complete target set", () -> | ||
| { | ||
| ArgumentCaptor<Set<MemberId>> targetMembers = ArgumentCaptor.forClass( (Class<Set<MemberId>>) expectedTargetSet.getClass() ); | ||
| verify( raftMock, atLeastOnce() ).setTargetMembershipSet( targetMembers.capture() ); | ||
| return targetMembers.getValue(); | ||
| }, equalTo( expectedTargetSet ), TIMEOUT_MS, MILLISECONDS ); | ||
| } | ||
| catch ( Throwable throwable ) | ||
| { | ||
| fail( throwable.getMessage() ); | ||
| } | ||
| return null; | ||
| }; | ||
| } | ||
| } |