diff --git a/java/google/registry/export/sheet/BUILD b/java/google/registry/export/sheet/BUILD index fb45794d8..92ac9ab0f 100644 --- a/java/google/registry/export/sheet/BUILD +++ b/java/google/registry/export/sheet/BUILD @@ -12,6 +12,7 @@ java_library( "//java/google/registry/model", "//java/google/registry/request", "//java/google/registry/request/auth", + "//java/google/registry/request/lock", "//java/google/registry/util", "//third_party/java/objectify:objectify-v4_1", "@com_google_api_client", diff --git a/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java b/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java index ce6975193..554b15d46 100644 --- a/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java +++ b/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java @@ -30,11 +30,11 @@ import com.google.appengine.api.taskqueue.TaskOptions.Method; import com.google.common.base.Optional; import com.google.gdata.util.ServiceException; import google.registry.config.RegistryConfig.Config; -import google.registry.model.server.Lock; import google.registry.request.Action; import google.registry.request.Parameter; import google.registry.request.Response; import google.registry.request.auth.Auth; +import google.registry.request.lock.LockHandler; import google.registry.util.FormattingLogger; import google.registry.util.NonFinalForTesting; import java.io.IOException; @@ -117,6 +117,7 @@ public class SyncRegistrarsSheetAction implements Runnable { @Inject @Config("sheetLockTimeout") Duration timeout; @Inject @Config("sheetRegistrarId") Optional idConfig; @Inject @Parameter("id") Optional idParam; + @Inject LockHandler lockHandler; @Inject SyncRegistrarsSheetAction() {} @Override @@ -146,7 +147,7 @@ public class SyncRegistrarsSheetAction implements Runnable { return null; } }; - if (!Lock.executeWithLocks(runner, null, timeout, sheetLockName)) { + if (!lockHandler.executeWithLocks(runner, null, timeout, sheetLockName)) { // If we fail to acquire the lock, it probably means lots of updates are happening at once, in // which case it should be safe to not bother. The task queue definition should *not* specify // max-concurrent-requests for this very reason. diff --git a/java/google/registry/request/BUILD b/java/google/registry/request/BUILD index b5af3372e..0974abe30 100644 --- a/java/google/registry/request/BUILD +++ b/java/google/registry/request/BUILD @@ -12,7 +12,7 @@ java_library( ), deps = [ "//java/google/registry/request/auth", - "//java/google/registry/security", + "//java/google/registry/request/lock", "//java/google/registry/util", "@com_google_appengine_api_1_0_sdk", "@com_google_auto_value", diff --git a/java/google/registry/request/RequestModule.java b/java/google/registry/request/RequestModule.java index 811124f43..a21b41720 100644 --- a/java/google/registry/request/RequestModule.java +++ b/java/google/registry/request/RequestModule.java @@ -27,6 +27,8 @@ import dagger.Provides; import google.registry.request.HttpException.BadRequestException; import google.registry.request.HttpException.UnsupportedMediaTypeException; import google.registry.request.auth.AuthResult; +import google.registry.request.lock.LockHandler; +import google.registry.request.lock.LockHandlerPassthrough; import java.io.IOException; import java.util.Map; import javax.servlet.http.HttpServletRequest; @@ -122,6 +124,11 @@ public final class RequestModule { } } + @Provides + static LockHandler provideLockHandler(LockHandlerPassthrough lockHandler) { + return lockHandler; + } + @Provides @JsonPayload @SuppressWarnings("unchecked") diff --git a/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java b/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java index b2abc1242..436d24a25 100644 --- a/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java +++ b/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java @@ -24,12 +24,12 @@ import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; import com.google.common.base.Optional; -import google.registry.model.server.Lock; import google.registry.testing.AppEngineRule; +import google.registry.testing.FakeLockHandler; import google.registry.testing.FakeResponse; -import java.util.concurrent.Callable; import javax.annotation.Nullable; import org.joda.time.Duration; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -48,16 +48,23 @@ public class SyncRegistrarsSheetActionTest { private final FakeResponse response = new FakeResponse(); private final SyncRegistrarsSheet syncRegistrarsSheet = mock(SyncRegistrarsSheet.class); + private SyncRegistrarsSheetAction action; + private void runAction(@Nullable String idConfig, @Nullable String idParam) { - SyncRegistrarsSheetAction action = new SyncRegistrarsSheetAction(); - action.response = response; - action.syncRegistrarsSheet = syncRegistrarsSheet; action.idConfig = Optional.fromNullable(idConfig); action.idParam = Optional.fromNullable(idParam); - action.timeout = Duration.standardHours(1); action.run(); } + @Before + public void setUp() { + action = new SyncRegistrarsSheetAction(); + action.response = response; + action.syncRegistrarsSheet = syncRegistrarsSheet; + action.timeout = Duration.standardHours(1); + action.lockHandler = new FakeLockHandler(true); + } + @Test public void testPost_withoutParamsOrSystemProperty_dropsTask() throws Exception { runAction(null, null); @@ -96,14 +103,8 @@ public class SyncRegistrarsSheetActionTest { @Test public void testPost_failToAquireLock_servletDoesNothingAndReturns() throws Exception { - String lockName = "Synchronize registrars sheet: foobar"; - Lock.executeWithLocks(new Callable() { - @Override - public Void call() throws Exception { - runAction(null, "foobar"); - return null; - } - }, null, Duration.standardHours(1), lockName); + action.lockHandler = new FakeLockHandler(false); + runAction(null, "foobar"); assertThat(response.getPayload()).startsWith("LOCKED"); verifyZeroInteractions(syncRegistrarsSheet); } diff --git a/javatests/google/registry/testing/BUILD b/javatests/google/registry/testing/BUILD index c92218e05..2b65a467b 100644 --- a/javatests/google/registry/testing/BUILD +++ b/javatests/google/registry/testing/BUILD @@ -29,6 +29,7 @@ java_library( "//java/google/registry/monitoring/whitebox", "//java/google/registry/pricing", "//java/google/registry/request", + "//java/google/registry/request/lock", "//java/google/registry/tmch", "//java/google/registry/util", "//java/google/registry/xml", diff --git a/javatests/google/registry/testing/FakeLockHandler.java b/javatests/google/registry/testing/FakeLockHandler.java new file mode 100644 index 000000000..44b11953a --- /dev/null +++ b/javatests/google/registry/testing/FakeLockHandler.java @@ -0,0 +1,57 @@ +// Copyright 2017 The Nomulus Authors. All Rights Reserved. +// +// 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 google.registry.testing; + +import static com.google.common.base.Throwables.throwIfUnchecked; + +import google.registry.request.lock.LockHandler; +import java.util.concurrent.Callable; +import javax.annotation.Nullable; +import org.joda.time.Duration; + +/** A fake {@link LockHandler} where user can control if lock acquisition succeeds. */ +public class FakeLockHandler implements LockHandler { + + private static final long serialVersionUID = 6437880915118738492L; + + boolean lockSucceeds = true; + + /** + * @param lockSucceeds if true - the lock acquisition will succeed and the callable will be + * called. If false, lock acquisition will fail and the caller isn't called. + */ + public FakeLockHandler(boolean lockSucceeds) { + this.lockSucceeds = lockSucceeds; + } + + @Override + public boolean executeWithLocks( + final Callable callable, + @Nullable String tld, + Duration leaseLength, + String... lockNames) { + if (!lockSucceeds) { + return false; + } + + try { + callable.call(); + } catch (Exception e) { + throwIfUnchecked(e); + throw new RuntimeException(e); + } + return true; + } +}