Skip to content

Commit 68830e6

Browse files
tulinkryVladimir Kotal
tulinkry
authored and
Vladimir Kotal
committed
using the project-repository map while being in write lock (#2451)
1 parent 3c29d04 commit 68830e6

File tree

2 files changed

+149
-57
lines changed

2 files changed

+149
-57
lines changed

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

Lines changed: 46 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -23,32 +23,10 @@
2323
*/
2424
package org.opengrok.indexer.configuration;
2525

26-
import org.apache.lucene.index.IndexReader;
27-
import org.apache.lucene.index.MultiReader;
28-
import org.apache.lucene.search.SearcherManager;
29-
import org.apache.lucene.store.AlreadyClosedException;
30-
import org.apache.lucene.store.Directory;
31-
import org.apache.lucene.store.FSDirectory;
32-
import org.opengrok.indexer.authorization.AuthorizationFramework;
33-
import org.opengrok.indexer.authorization.AuthorizationStack;
34-
import org.opengrok.indexer.history.HistoryGuru;
35-
import org.opengrok.indexer.history.RepositoryInfo;
36-
import org.opengrok.indexer.index.Filter;
37-
import org.opengrok.indexer.index.IgnoredNames;
38-
import org.opengrok.indexer.index.IndexDatabase;
39-
import org.opengrok.indexer.logger.LoggerFactory;
40-
import org.opengrok.indexer.util.CtagsUtil;
41-
import org.opengrok.indexer.util.ForbiddenSymlinkException;
42-
import org.opengrok.indexer.util.PathUtils;
43-
import org.opengrok.indexer.web.Prefix;
44-
import org.opengrok.indexer.web.Statistics;
45-
import org.opengrok.indexer.web.messages.Message;
46-
import org.opengrok.indexer.web.messages.MessagesContainer;
47-
import org.opengrok.indexer.web.messages.MessagesContainer.AcceptedMessage;
26+
import static org.opengrok.indexer.configuration.Configuration.makeXMLStringAsConfiguration;
27+
import static org.opengrok.indexer.util.ClassUtil.getFieldValue;
28+
import static org.opengrok.indexer.util.ClassUtil.setFieldValue;
4829

49-
import javax.ws.rs.client.ClientBuilder;
50-
import javax.ws.rs.client.Entity;
51-
import javax.ws.rs.core.Response;
5230
import java.io.File;
5331
import java.io.FileNotFoundException;
5432
import java.io.IOException;
@@ -71,10 +49,31 @@
7149
import java.util.logging.Level;
7250
import java.util.logging.Logger;
7351
import java.util.stream.Collectors;
74-
75-
import static org.opengrok.indexer.configuration.Configuration.makeXMLStringAsConfiguration;
76-
import static org.opengrok.indexer.util.ClassUtil.getFieldValue;
77-
import static org.opengrok.indexer.util.ClassUtil.setFieldValue;
52+
import javax.ws.rs.client.ClientBuilder;
53+
import javax.ws.rs.client.Entity;
54+
import javax.ws.rs.core.Response;
55+
import org.apache.lucene.index.IndexReader;
56+
import org.apache.lucene.index.MultiReader;
57+
import org.apache.lucene.search.SearcherManager;
58+
import org.apache.lucene.store.AlreadyClosedException;
59+
import org.apache.lucene.store.Directory;
60+
import org.apache.lucene.store.FSDirectory;
61+
import org.opengrok.indexer.authorization.AuthorizationFramework;
62+
import org.opengrok.indexer.authorization.AuthorizationStack;
63+
import org.opengrok.indexer.history.HistoryGuru;
64+
import org.opengrok.indexer.history.RepositoryInfo;
65+
import org.opengrok.indexer.index.Filter;
66+
import org.opengrok.indexer.index.IgnoredNames;
67+
import org.opengrok.indexer.index.IndexDatabase;
68+
import org.opengrok.indexer.logger.LoggerFactory;
69+
import org.opengrok.indexer.util.CtagsUtil;
70+
import org.opengrok.indexer.util.ForbiddenSymlinkException;
71+
import org.opengrok.indexer.util.PathUtils;
72+
import org.opengrok.indexer.web.Prefix;
73+
import org.opengrok.indexer.web.Statistics;
74+
import org.opengrok.indexer.web.messages.Message;
75+
import org.opengrok.indexer.web.messages.MessagesContainer;
76+
import org.opengrok.indexer.web.messages.MessagesContainer.AcceptedMessage;
7877

7978
/**
8079
* The RuntimeEnvironment class is used as a placeholder for the current
@@ -1432,21 +1431,25 @@ public void signalTorefreshSearcherManagers(List<String> subFiles, String host)
14321431
* otherwise it is just a simple project.
14331432
*/
14341433
private void generateProjectRepositoriesMap() throws IOException {
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+
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+
}
14461446

1447-
if ((proj = Project.getProject(repoPath)) != null) {
1448-
List<RepositoryInfo> values = repository_map.computeIfAbsent(proj, k -> new ArrayList<>());
1449-
values.add(r);
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+
}
14501453
}
14511454
}
14521455
}

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

Lines changed: 103 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,33 +22,43 @@
2222
*/
2323
package org.opengrok.web.api.v1.controller;
2424

25+
import static org.junit.Assert.assertEquals;
26+
import static org.mockito.Mockito.reset;
27+
import static org.mockito.Mockito.verify;
28+
29+
import java.io.IOException;
30+
import java.nio.file.Paths;
31+
import java.util.ArrayList;
32+
import java.util.List;
33+
import java.util.Map;
34+
import java.util.TreeMap;
35+
import java.util.concurrent.CountDownLatch;
36+
import javax.servlet.http.HttpServletRequest;
37+
import javax.ws.rs.client.Entity;
38+
import javax.ws.rs.core.Application;
39+
import javax.ws.rs.core.Response;
40+
import org.apache.commons.io.FileUtils;
2541
import org.glassfish.jersey.internal.inject.AbstractBinder;
2642
import org.glassfish.jersey.server.ResourceConfig;
2743
import org.glassfish.jersey.test.JerseyTest;
2844
import org.junit.Assert;
45+
import org.junit.Rule;
2946
import org.junit.Test;
3047
import org.mockito.Mock;
3148
import org.mockito.MockitoAnnotations;
49+
import org.opengrok.indexer.condition.ConditionalRun;
50+
import org.opengrok.indexer.condition.ConditionalRunRule;
51+
import org.opengrok.indexer.condition.RepositoryInstalled;
3252
import org.opengrok.indexer.configuration.Configuration;
53+
import org.opengrok.indexer.configuration.Project;
3354
import org.opengrok.indexer.configuration.RuntimeEnvironment;
55+
import org.opengrok.indexer.history.HistoryGuru;
56+
import org.opengrok.indexer.history.RepositoryInfo;
57+
import org.opengrok.indexer.util.TestRepository;
3458
import org.opengrok.indexer.web.DummyHttpServletRequest;
3559
import org.opengrok.indexer.web.PageConfig;
3660
import org.opengrok.web.api.v1.suggester.provider.service.SuggesterService;
3761

38-
import javax.servlet.http.HttpServletRequest;
39-
import javax.ws.rs.client.Entity;
40-
import javax.ws.rs.core.Application;
41-
import javax.ws.rs.core.Response;
42-
43-
import java.util.concurrent.CountDownLatch;
44-
import java.util.concurrent.ExecutorService;
45-
import java.util.concurrent.Executors;
46-
import java.util.concurrent.TimeUnit;
47-
48-
import static org.junit.Assert.assertEquals;
49-
import static org.mockito.Mockito.reset;
50-
import static org.mockito.Mockito.verify;
51-
5262
public class ConfigurationControllerTest extends JerseyTest {
5363

5464
private RuntimeEnvironment env = RuntimeEnvironment.getInstance();
@@ -291,4 +301,83 @@ public void run() {
291301
// Revert the value back to the default.
292302
env.setHitsPerPage(origValue);
293303
}
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+
}
294383
}

0 commit comments

Comments
 (0)