Skip to content

Commit 5f6ae75

Browse files
tulinkryVladimir Kotal
authored andcommitted
Better synchronization of RE (#2501)
fixes #2496
1 parent e4acabf commit 5f6ae75

File tree

4 files changed

+223
-118
lines changed

4 files changed

+223
-118
lines changed

opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/RuntimeEnvironment.java

Lines changed: 34 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1426,30 +1426,28 @@ public void signalTorefreshSearcherManagers(List<String> subFiles, String host)
14261426

14271427
/**
14281428
* Generate a TreeMap of projects with corresponding repository information.
1429-
*
1429+
* <p>
14301430
* Project with some repository information is considered as a repository
14311431
* otherwise it is just a simple project.
14321432
*/
14331433
private void generateProjectRepositoriesMap() throws IOException {
1434-
synchronized (repository_map) {
1435-
repository_map.clear();
1436-
for (RepositoryInfo r : getRepositories()) {
1437-
Project proj;
1438-
String repoPath;
1439-
try {
1440-
repoPath = PathUtils.getPathRelativeToSourceRoot(
1441-
new File(r.getDirectoryName()), 0);
1442-
} catch (ForbiddenSymlinkException e) {
1443-
LOGGER.log(Level.FINER, e.getMessage());
1444-
continue;
1445-
}
1434+
repository_map.clear();
1435+
for (RepositoryInfo r : getRepositories()) {
1436+
Project proj;
1437+
String repoPath;
1438+
try {
1439+
repoPath = PathUtils.getPathRelativeToSourceRoot(
1440+
new File(r.getDirectoryName()), 0);
1441+
} catch (ForbiddenSymlinkException e) {
1442+
LOGGER.log(Level.FINER, e.getMessage());
1443+
continue;
1444+
}
14461445

1447-
if ((proj = Project.getProject(repoPath)) != null) {
1448-
List<RepositoryInfo> values = repository_map.computeIfAbsent(proj, k -> new ArrayList<>());
1449-
// the map is held under the lock because the next call to
1450-
// values.add(r) which should not be called from multiple threads at the same time
1451-
values.add(r);
1452-
}
1446+
if ((proj = Project.getProject(repoPath)) != null) {
1447+
List<RepositoryInfo> values = repository_map.computeIfAbsent(proj, k -> new ArrayList<>());
1448+
// the map is held under the lock because the next call to
1449+
// values.add(r) which should not be called from multiple threads at the same time
1450+
values.add(r);
14531451
}
14541452
}
14551453
}
@@ -1502,11 +1500,12 @@ public void setConfiguration(Configuration configuration, boolean interactive) {
15021500

15031501
/**
15041502
* Sets the configuration and performs necessary actions.
1503+
*
15051504
* @param configuration new configuration
1506-
* @param subFileList list of repositories
1507-
* @param interactive true if in interactive mode
1505+
* @param subFileList list of repositories
1506+
* @param interactive true if in interactive mode
15081507
*/
1509-
public void setConfiguration(Configuration configuration, List<String> subFileList, boolean interactive) {
1508+
public synchronized void setConfiguration(Configuration configuration, List<String> subFileList, boolean interactive) {
15101509
try {
15111510
configLock.writeLock().lock();
15121511
this.configuration = configuration;
@@ -1517,32 +1516,29 @@ public void setConfiguration(Configuration configuration, List<String> subFileLi
15171516
// HistoryGuru constructor needs environment properties so no locking is done here.
15181517
HistoryGuru histGuru = HistoryGuru.getInstance();
15191518

1520-
try {
1521-
generateProjectRepositoriesMap();
1522-
} catch (IOException ex) {
1523-
LOGGER.log(Level.SEVERE, "Cannot generate project - repository map", ex);
1524-
}
1525-
1526-
populateGroups(getGroups(), new TreeSet<>(getProjects().values()));
1527-
15281519
// Set the working repositories in HistoryGuru.
15291520
if (subFileList != null) {
15301521
histGuru.invalidateRepositories(
1531-
configuration.getRepositories(), subFileList, interactive);
1522+
getRepositories(), subFileList, interactive);
15321523
} else {
1533-
histGuru.invalidateRepositories(configuration.getRepositories(),
1524+
histGuru.invalidateRepositories(getRepositories(),
15341525
interactive);
15351526
}
15361527

1528+
// The invalidation of repositories above might have excluded some
1529+
// repositories in HistoryGuru so the configuration needs to reflect that.
1530+
setRepositories(new ArrayList<>(histGuru.getRepositories()));
1531+
1532+
// generate repository map is dependent on getRepositories()
15371533
try {
1538-
configLock.writeLock().lock();
1539-
// The invalidation of repositories above might have excluded some
1540-
// repositories in HistoryGuru so the configuration needs to reflect that.
1541-
configuration.setRepositories(new ArrayList<>(histGuru.getRepositories()));
1542-
} finally {
1543-
configLock.writeLock().unlock();
1534+
generateProjectRepositoriesMap();
1535+
} catch (IOException ex) {
1536+
LOGGER.log(Level.SEVERE, "Cannot generate project - repository map", ex);
15441537
}
15451538

1539+
// populate groups is dependent on repositories map
1540+
populateGroups(getGroups(), new TreeSet<>(getProjects().values()));
1541+
15461542
includeFiles.reloadIncludeFiles();
15471543
}
15481544

opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryGuru.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -977,7 +977,7 @@ public void run() {
977977
repositories.clear();
978978
newrepos.forEach((_key, repo) -> { putRepository(repo); });
979979

980-
elapsed.report(LOGGER, "done invalidating repositories");
980+
elapsed.report(LOGGER, String.format("done invalidating %d repositories", newrepos.size()));
981981
}
982982

983983
/**
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
package org.opengrok.web.api.v1.controller;
2+
3+
import java.nio.file.Paths;
4+
import java.util.ArrayList;
5+
import java.util.LinkedList;
6+
import java.util.List;
7+
import java.util.Map;
8+
import java.util.TreeMap;
9+
import java.util.concurrent.ExecutionException;
10+
import java.util.concurrent.ExecutorService;
11+
import java.util.concurrent.Executors;
12+
import java.util.concurrent.Future;
13+
import java.util.concurrent.TimeUnit;
14+
import javax.ws.rs.client.Entity;
15+
import javax.ws.rs.core.Application;
16+
import javax.ws.rs.core.Response;
17+
import org.apache.commons.io.FileUtils;
18+
import org.glassfish.jersey.internal.inject.AbstractBinder;
19+
import org.glassfish.jersey.server.ResourceConfig;
20+
import org.glassfish.jersey.test.JerseyTest;
21+
import org.junit.After;
22+
import org.junit.Assert;
23+
import org.junit.Before;
24+
import org.junit.Rule;
25+
import org.junit.Test;
26+
import org.mockito.Mock;
27+
import org.mockito.MockitoAnnotations;
28+
import org.opengrok.indexer.condition.ConditionalRun;
29+
import org.opengrok.indexer.condition.ConditionalRunRule;
30+
import org.opengrok.indexer.condition.RepositoryInstalled;
31+
import org.opengrok.indexer.configuration.Project;
32+
import org.opengrok.indexer.configuration.RuntimeEnvironment;
33+
import org.opengrok.indexer.history.HistoryGuru;
34+
import org.opengrok.indexer.history.RepositoryInfo;
35+
import org.opengrok.indexer.util.TestRepository;
36+
import org.opengrok.web.api.v1.suggester.provider.service.SuggesterService;
37+
38+
@ConditionalRun(RepositoryInstalled.GitInstalled.class)
39+
public class ConcurrentConfigurationControllerTest extends JerseyTest {
40+
41+
private static final int PROJECTS_COUNT = 20;
42+
private static final int THREAD_COUNT = Math.max(30, Runtime.getRuntime().availableProcessors() * 2);
43+
private static final int TASK_COUNT = 100;
44+
45+
@Rule
46+
public ConditionalRunRule rule = new ConditionalRunRule();
47+
48+
@Mock
49+
private SuggesterService suggesterService;
50+
51+
private RuntimeEnvironment env = RuntimeEnvironment.getInstance();
52+
53+
TestRepository repository;
54+
String origSourceRootPath;
55+
String origDataRootPath;
56+
Map<String, Project> origProjects;
57+
List<RepositoryInfo> origRepositories;
58+
59+
@Override
60+
protected Application configure() {
61+
MockitoAnnotations.initMocks(this);
62+
return new ResourceConfig(ConfigurationController.class)
63+
.register(new AbstractBinder() {
64+
@Override
65+
protected void configure() {
66+
bind(suggesterService).to(SuggesterService.class);
67+
}
68+
});
69+
}
70+
71+
@Before
72+
public void setUp() throws Exception {
73+
super.setUp();
74+
origSourceRootPath = env.getSourceRootPath();
75+
origDataRootPath = env.getDataRootPath();
76+
origProjects = env.getProjects();
77+
origRepositories = env.getRepositories();
78+
79+
// prepare test repository
80+
repository = new TestRepository();
81+
repository.create(HistoryGuru.class.getResourceAsStream("repositories.zip"));
82+
83+
env.setSourceRoot(repository.getSourceRoot());
84+
env.setDataRoot(repository.getDataRoot());
85+
86+
87+
List<RepositoryInfo> repositoryInfos = new ArrayList<>();
88+
Map<String, Project> projects = new TreeMap<>();
89+
90+
/*
91+
* Prepare PROJECTS_COUNT git repositories, named project-{i} in the test repositories directory.
92+
*/
93+
for (int i = 0; i < PROJECTS_COUNT; i++) {
94+
Project project = new Project();
95+
project.setName("project-" + i);
96+
project.setPath("/project-" + i);
97+
RepositoryInfo repo = new RepositoryInfo();
98+
repo.setDirectoryNameRelative("/project-" + i);
99+
100+
projects.put("project-" + i, project);
101+
repositoryInfos.add(repo);
102+
103+
// create the repository
104+
FileUtils.copyDirectory(
105+
Paths.get(repository.getSourceRoot(), "git").toFile(),
106+
Paths.get(repository.getSourceRoot(), "project-" + i).toFile()
107+
);
108+
}
109+
110+
env.setRepositories(repositoryInfos);
111+
env.setProjects(projects);
112+
}
113+
114+
@After
115+
public void tearDown() throws Exception {
116+
super.tearDown();
117+
repository.destroy();
118+
119+
env.setProjects(origProjects);
120+
env.setRepositories(origRepositories);
121+
env.setSourceRoot(origSourceRootPath);
122+
env.setDataRoot(origDataRootPath);
123+
}
124+
125+
@Test
126+
public void testConcurrentConfigurationReloads() throws InterruptedException, ExecutionException {
127+
final ExecutorService threadPool = Executors.newFixedThreadPool(THREAD_COUNT);
128+
List<Future<?>> futures = new LinkedList<>();
129+
130+
/*
131+
* Now run setting a value in parallel which triggers configuration reload.
132+
*/
133+
for (int i = 0; i < TASK_COUNT; i++) {
134+
futures.add(threadPool.submit(() -> {
135+
Response put = target("configuration")
136+
.path("projectsEnabled")
137+
.request()
138+
.put(Entity.text("true"));
139+
140+
Assert.assertEquals(204, put.getStatus());
141+
142+
assertTestedProjects();
143+
}));
144+
}
145+
146+
threadPool.shutdown();
147+
threadPool.awaitTermination(1, TimeUnit.MINUTES);
148+
149+
for (Future<?> future : futures) {
150+
// calling get on a future will rethrow the exceptions in its thread (i. e. assertions in this case)
151+
future.get();
152+
}
153+
}
154+
155+
@Test
156+
public void testConcurrentCInvalidateRepositories() throws InterruptedException, ExecutionException {
157+
final ExecutorService threadPool = Executors.newFixedThreadPool(THREAD_COUNT);
158+
final List<RepositoryInfo> repositoryInfos = env.getRepositories();
159+
List<Future<?>> futures = new LinkedList<>();
160+
161+
/*
162+
* Now run apply config in parallel
163+
*/
164+
for (int i = 0; i < TASK_COUNT; i++) {
165+
futures.add(threadPool.submit(() -> {
166+
env.applyConfig(false, false);
167+
assertTestedProjects();
168+
}));
169+
}
170+
171+
threadPool.shutdown();
172+
threadPool.awaitTermination(1, TimeUnit.MINUTES);
173+
174+
for (Future<?> future : futures) {
175+
// calling get on a future will rethrow the exceptions in its thread (i. e. assertions in this case)
176+
future.get();
177+
}
178+
}
179+
180+
private void assertTestedProjects() {
181+
Assert.assertEquals(PROJECTS_COUNT, env.getProjects().size());
182+
Assert.assertEquals(PROJECTS_COUNT, env.getRepositories().size());
183+
Assert.assertEquals(PROJECTS_COUNT, env.getProjectRepositoriesMap().size());
184+
env.getProjectRepositoriesMap().forEach((project, repositories) -> {
185+
Assert.assertEquals(1, repositories.size());
186+
});
187+
}
188+
}

opengrok-web/src/test/java/org/opengrok/web/api/v1/controller/ConfigurationControllerTest.java

Lines changed: 0 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -301,83 +301,4 @@ public void run() {
301301
// Revert the value back to the default.
302302
env.setHitsPerPage(origValue);
303303
}
304-
305-
@Rule
306-
public ConditionalRunRule rule = new ConditionalRunRule();
307-
308-
@Test
309-
@ConditionalRun(RepositoryInstalled.GitInstalled.class)
310-
public void testConcurrentConfigurationReloads() throws InterruptedException, IOException {
311-
final String origSourceRootPath = env.getSourceRootPath();
312-
final String origDataRootPath = env.getDataRootPath();
313-
final Map<String, Project> origProjects = env.getProjects();
314-
final List<RepositoryInfo> origRepositories = env.getRepositories();
315-
316-
final int nThreads = Math.max(40, Runtime.getRuntime().availableProcessors() * 2);
317-
final int nProjects = 20;
318-
319-
// prepare test repository
320-
TestRepository repository = new TestRepository();
321-
repository.create(HistoryGuru.class.getResourceAsStream("repositories.zip"));
322-
323-
env.setSourceRoot(repository.getSourceRoot());
324-
env.setDataRoot(repository.getDataRoot());
325-
326-
final CountDownLatch latch = new CountDownLatch(nThreads);
327-
328-
List<RepositoryInfo> repositoryInfos = new ArrayList<>();
329-
Map<String, Project> projects = new TreeMap<>();
330-
331-
/*
332-
* Prepare nProjects git repositories, named project-{i} in the test repositories directory.
333-
*/
334-
for (int i = 0; i < nProjects; i++) {
335-
Project project = new Project();
336-
project.setName("project-" + i);
337-
project.setPath("/project-" + i);
338-
RepositoryInfo repo = new RepositoryInfo();
339-
repo.setDirectoryNameRelative("/project-" + i);
340-
341-
projects.put("project-" + i, project);
342-
repositoryInfos.add(repo);
343-
344-
// create the repository
345-
FileUtils.copyDirectory(
346-
Paths.get(repository.getSourceRoot(), "git").toFile(),
347-
Paths.get(repository.getSourceRoot(), "project-" + i).toFile()
348-
);
349-
}
350-
351-
env.setRepositories(repositoryInfos);
352-
env.setProjects(projects);
353-
354-
/*
355-
* Now run setting a value in parallel, which triggers configuration reload.
356-
*/
357-
for (int i = 0; i < nThreads; i++) {
358-
new Thread(() -> {
359-
Response put = target("configuration")
360-
.path("projectsEnabled")
361-
.request()
362-
.put(Entity.text("true"));
363-
Assert.assertEquals(204, put.getStatus());
364-
latch.countDown();
365-
}).start();
366-
}
367-
368-
latch.await();
369-
370-
Assert.assertEquals(nProjects, env.getProjects().size());
371-
Assert.assertEquals(nProjects, env.getProjectRepositoriesMap().size());
372-
env.getProjectRepositoriesMap().forEach((project, repositories) -> {
373-
Assert.assertEquals(1, repositories.size());
374-
});
375-
376-
repository.destroy();
377-
378-
env.setProjects(origProjects);
379-
env.setRepositories(origRepositories);
380-
env.setSourceRoot(origSourceRootPath);
381-
env.setDataRoot(origDataRootPath);
382-
}
383304
}

0 commit comments

Comments
 (0)