From ed561804ebade686ed4f7372d7a3b2f0b7cb4e29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Tue, 10 Oct 2017 16:29:58 +0200 Subject: [PATCH] Handle concurrent creation of the same project consistently When two requests create the same project, one is expected to succeed with SC_CREATED and another to fail with SC_CONFLICT. This works well when these two requests are executed sequentially. However, when the two requests were executed concurrently, the result was less predictable: * it could be as described above or * one of the requests would randomly fail with an SC_INTERNAL_SERVER_ERROR. Add a test for concurrent project creation. Execute concurrent project creation several times to increase probability of reproducing the issue in the code as it was before this change. Introduce ProjectNameLockManager for locking project names and use it from the CreateProject REST API to synchronize on the same project name. The default implementation of the ProjectNameLockManager uses an in-memory (guava) cache of locks. This is a good implementation for single instance Gerrit servers. Exposes the ProjectNameLockManager as an extension point so that a plugin can provide own implementation. The idea is that the high-availability plugin provides own project name locking based on JGroups cluster wide locking [1] or a kind of file system based locking and thus ensure consistent behaviour when running in active-active mode. Further, the delete-project plugin can now also make use of the ProjectNameLockManager in order to prevent (re)creation of the same project while it is being deleted. [1] http://jgroups.org/manual/#LockService Change-Id: Idf52850e02adf823a54537b975631c3c3e4737d2 --- .../rest/project/CreateProjectIT.java | 30 +++++++++ .../java/com/google/gerrit/pgm/Daemon.java | 2 + .../server/config/GerritGlobalModule.java | 2 + .../gerrit/server/project/CreateProject.java | 36 +++++++---- .../DefaultProjectNameLockManager.java | 61 +++++++++++++++++++ .../project/ProjectNameLockManager.java | 22 +++++++ .../gerrit/testutil/InMemoryModule.java | 2 + .../gerrit/httpd/WebAppInitializer.java | 2 + 8 files changed, 144 insertions(+), 13 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultProjectNameLockManager.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectNameLockManager.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java index 0409fbca62..b47b51a5a8 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java @@ -43,6 +43,12 @@ import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.project.ProjectState; import java.util.Collections; import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import org.apache.http.HttpStatus; import org.apache.http.message.BasicHeader; import org.eclipse.jgit.lib.Constants; @@ -84,6 +90,30 @@ public class CreateProjectIT extends AbstractDaemonTest { .assertPreconditionFailed(); } + @Test + public void createSameProjectFromTwoConcurrentRequests() throws Exception { + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + for (int i = 0; i < 10; i++) { + String newProjectName = name("foo" + i); + CyclicBarrier sync = new CyclicBarrier(2); + Callable createProjectFoo = + () -> { + sync.await(); + return adminRestSession.put("/projects/" + newProjectName); + }; + + Future r1 = executor.submit(createProjectFoo); + Future r2 = executor.submit(createProjectFoo); + assertThat(ImmutableList.of(r1.get().getStatusCode(), r2.get().getStatusCode())) + .containsAllOf(HttpStatus.SC_CREATED, HttpStatus.SC_CONFLICT); + } + } finally { + executor.shutdown(); + executor.awaitTermination(5, TimeUnit.SECONDS); + } + } + @Test @UseLocalDisk public void createProjectHttpWithUnreasonableName_BadRequest() throws Exception { diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index 1a5600f9d6..4c44ef33c2 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -85,6 +85,7 @@ import com.google.gerrit.server.plugins.PluginGuiceEnvironment; import com.google.gerrit.server.plugins.PluginModule; import com.google.gerrit.server.plugins.PluginRestApiModule; import com.google.gerrit.server.project.DefaultPermissionBackendModule; +import com.google.gerrit.server.project.DefaultProjectNameLockManager; import com.google.gerrit.server.schema.DataSourceProvider; import com.google.gerrit.server.schema.InMemoryAccountPatchReviewStore; import com.google.gerrit.server.schema.JdbcAccountPatchReviewStore; @@ -474,6 +475,7 @@ public class Daemon extends SiteProgram { modules.add(testSysModule); } modules.add(new LocalMergeSuperSetComputation.Module()); + modules.add(new DefaultProjectNameLockManager.Module()); return cfgInjector.createChildInjector(modules); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 32670804f2..7403b1c908 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -164,6 +164,7 @@ import com.google.gerrit.server.project.AccessControlModule; import com.google.gerrit.server.project.CommentLinkProvider; import com.google.gerrit.server.project.PermissionCollection; import com.google.gerrit.server.project.ProjectCacheImpl; +import com.google.gerrit.server.project.ProjectNameLockManager; import com.google.gerrit.server.project.ProjectNode; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.SectionSortCache; @@ -379,6 +380,7 @@ public class GerritGlobalModule extends FactoryModule { DynamicSet.setOf(binder(), AssigneeValidationListener.class); DynamicSet.setOf(binder(), ActionVisitor.class); DynamicItem.itemOf(binder(), MergeSuperSetComputation.class); + DynamicItem.itemOf(binder(), ProjectNameLockManager.class); DynamicMap.mapOf(binder(), MailFilter.class); bind(MailFilter.class).annotatedWith(Exports.named("ListMailFilter")).to(ListMailFilter.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateProject.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateProject.java index 9b355f1921..70cb266f0f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateProject.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateProject.java @@ -31,6 +31,7 @@ import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ProjectInfo; import com.google.gerrit.extensions.events.NewProjectCreatedListener; +import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; @@ -65,6 +66,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.concurrent.locks.Lock; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.CommitBuilder; @@ -103,6 +105,7 @@ public class CreateProject implements RestModifyView identifiedUser; private final Provider putConfig; private final AllProjectsName allProjects; + private final DynamicItem lockManager; private final String name; @Inject @@ -123,6 +126,7 @@ public class CreateProject implements RestModifyView identifiedUser, Provider putConfig, AllProjectsName allProjects, + DynamicItem lockManager, @Assisted String name) { this.projectsCollection = projectsCollection; this.groupsCollection = groupsCollection; @@ -140,6 +144,7 @@ public class CreateProject implements RestModifyView lockCache = + CacheBuilder.newBuilder() + .maximumSize(1024) + .expireAfterAccess(5, TimeUnit.MINUTES) + .build( + new CacheLoader() { + @Override + public Lock load(NameKey key) throws Exception { + return new ReentrantLock(); + } + }); + + @Override + public Lock getLock(NameKey name) { + try { + return lockCache.get(name); + } catch (ExecutionException e) { + throw new RuntimeException(e); + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectNameLockManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectNameLockManager.java new file mode 100644 index 0000000000..4666c32fa0 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectNameLockManager.java @@ -0,0 +1,22 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.project; + +import com.google.gerrit.reviewdb.client.Project; +import java.util.concurrent.locks.Lock; + +public interface ProjectNameLockManager { + public Lock getLock(Project.NameKey name); +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java index 2dca864082..d542bc332a 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java @@ -68,6 +68,7 @@ import com.google.gerrit.server.patch.DiffExecutor; import com.google.gerrit.server.plugins.PluginRestApiModule; import com.google.gerrit.server.plugins.ServerInformationImpl; import com.google.gerrit.server.project.DefaultPermissionBackendModule; +import com.google.gerrit.server.project.DefaultProjectNameLockManager; import com.google.gerrit.server.schema.DataSourceType; import com.google.gerrit.server.schema.InMemoryAccountPatchReviewStore; import com.google.gerrit.server.schema.NotesMigrationSchemaFactory; @@ -249,6 +250,7 @@ public class InMemoryModule extends FactoryModule { bind(ServerInformationImpl.class); bind(ServerInformation.class).to(ServerInformationImpl.class); install(new PluginRestApiModule()); + install(new DefaultProjectNameLockManager.Module()); } @Provides diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index e2f3dcf87e..45270faa88 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -66,6 +66,7 @@ import com.google.gerrit.server.plugins.PluginGuiceEnvironment; import com.google.gerrit.server.plugins.PluginModule; import com.google.gerrit.server.plugins.PluginRestApiModule; import com.google.gerrit.server.project.DefaultPermissionBackendModule; +import com.google.gerrit.server.project.DefaultProjectNameLockManager; import com.google.gerrit.server.schema.DataSourceModule; import com.google.gerrit.server.schema.DataSourceProvider; import com.google.gerrit.server.schema.DataSourceType; @@ -370,6 +371,7 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi modules.add(new ChangeCleanupRunner.Module()); modules.add(new AccountDeactivator.Module()); modules.addAll(LibModuleLoader.loadModules(cfgInjector)); + modules.add(new DefaultProjectNameLockManager.Module()); return cfgInjector.createChildInjector(modules); }