Merge "Handle concurrent creation of the same project consistently"

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<RestResponse> createProjectFoo =
+            () -> {
+              sync.await();
+              return adminRestSession.put("/projects/" + newProjectName);
+            };
+
+        Future<RestResponse> r1 = executor.submit(createProjectFoo);
+        Future<RestResponse> 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 {

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);
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -163,6 +163,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;
@@ -376,6 +377,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);

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<TopLevelResource, ProjectIn
   private final Provider<IdentifiedUser> identifiedUser;
   private final Provider<PutConfig> putConfig;
   private final AllProjectsName allProjects;
+  private final DynamicItem<ProjectNameLockManager> lockManager;
   private final String name;
 
   @Inject
@@ -123,6 +126,7 @@ public class CreateProject implements RestModifyView<TopLevelResource, ProjectIn
       Provider<IdentifiedUser> identifiedUser,
       Provider<PutConfig> putConfig,
       AllProjectsName allProjects,
+      DynamicItem<ProjectNameLockManager> lockManager,
       @Assisted String name) {
     this.projectsCollection = projectsCollection;
     this.groupsCollection = groupsCollection;
@@ -140,6 +144,7 @@ public class CreateProject implements RestModifyView<TopLevelResource, ProjectIn
     this.identifiedUser = identifiedUser;
     this.putConfig = putConfig;
     this.allProjects = allProjects;
+    this.lockManager = lockManager;
     this.name = name;
   }
 
@@ -192,22 +197,27 @@ public class CreateProject implements RestModifyView<TopLevelResource, ProjectIn
       throw new BadRequestException(e.getMessage());
     }
 
-    for (ProjectCreationValidationListener l : projectCreationValidationListeners) {
+    Lock nameLock = lockManager.get().getLock(args.getProject());
-      try {
+    nameLock.lock();
-        l.validateNewProject(args);
+    try {
-      } catch (ValidationException e) {
+      for (ProjectCreationValidationListener l : projectCreationValidationListeners) {
-        throw new ResourceConflictException(e.getMessage(), e);
+        try {
+          l.validateNewProject(args);
+        } catch (ValidationException e) {
+          throw new ResourceConflictException(e.getMessage(), e);
+        }
       }
-    }
 
-    ProjectState projectState = createProject(args);
+      ProjectState projectState = createProject(args);
-    if (input.pluginConfigValues != null) {
+      if (input.pluginConfigValues != null) {
-      ConfigInput in = new ConfigInput();
+        ConfigInput in = new ConfigInput();
-      in.pluginConfigValues = input.pluginConfigValues;
+        in.pluginConfigValues = input.pluginConfigValues;
-      putConfig.get().apply(projectState, in);
+        putConfig.get().apply(projectState, in);
+      }
+      return Response.created(json.format(projectState));
+    } finally {
+      nameLock.unlock();
     }
-
-    return Response.created(json.format(projectState));
   }
 
   private ProjectState createProject(CreateProjectArgs args)

gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultProjectNameLockManager.java

@@ -0,0 +1,61 @@
+// 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.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.inject.AbstractModule;
+import com.google.inject.Singleton;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+@Singleton
+public class DefaultProjectNameLockManager implements ProjectNameLockManager {
+
+  public static class Module extends AbstractModule {
+    @Override
+    protected void configure() {
+      DynamicItem.bind(binder(), ProjectNameLockManager.class)
+          .to(DefaultProjectNameLockManager.class);
+    }
+  }
+
+  LoadingCache<Project.NameKey, Lock> lockCache =
+      CacheBuilder.newBuilder()
+          .maximumSize(1024)
+          .expireAfterAccess(5, TimeUnit.MINUTES)
+          .build(
+              new CacheLoader<Project.NameKey, Lock>() {
+                @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);
+    }
+  }
+}

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);
+}

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

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);
   }