Create an injectable LockHandler

We create an injectable LockHandler that just calls the static
Lock.executeWithLocks function.

I'm not sure what's the correct place to put the LockHandler. I think
model/server is only appropriate for the actual datastore lock. This is a "per request" lock, so maybe request/lock?

-----------------------------

This is the initial step in adding the "lock implicitly released on request death" feature, but it's also useful on its own - easier to test Actions when we can use a fake lock.

To keep this CL simple, we keep using the old Lock as is in most places. We just choose a single example to convert to LockHandler to showcase it. Converting all other uses will be in a subsequent CL.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=167357564
This commit is contained in:
guyben 2017-09-01 21:32:37 -07:00 committed by jianglai
parent 67276888d2
commit 978149e677
7 changed files with 85 additions and 17 deletions

View file

@ -12,6 +12,7 @@ java_library(
"//java/google/registry/model", "//java/google/registry/model",
"//java/google/registry/request", "//java/google/registry/request",
"//java/google/registry/request/auth", "//java/google/registry/request/auth",
"//java/google/registry/request/lock",
"//java/google/registry/util", "//java/google/registry/util",
"//third_party/java/objectify:objectify-v4_1", "//third_party/java/objectify:objectify-v4_1",
"@com_google_api_client", "@com_google_api_client",

View file

@ -30,11 +30,11 @@ import com.google.appengine.api.taskqueue.TaskOptions.Method;
import com.google.common.base.Optional; import com.google.common.base.Optional;
import com.google.gdata.util.ServiceException; import com.google.gdata.util.ServiceException;
import google.registry.config.RegistryConfig.Config; import google.registry.config.RegistryConfig.Config;
import google.registry.model.server.Lock;
import google.registry.request.Action; import google.registry.request.Action;
import google.registry.request.Parameter; import google.registry.request.Parameter;
import google.registry.request.Response; import google.registry.request.Response;
import google.registry.request.auth.Auth; import google.registry.request.auth.Auth;
import google.registry.request.lock.LockHandler;
import google.registry.util.FormattingLogger; import google.registry.util.FormattingLogger;
import google.registry.util.NonFinalForTesting; import google.registry.util.NonFinalForTesting;
import java.io.IOException; import java.io.IOException;
@ -117,6 +117,7 @@ public class SyncRegistrarsSheetAction implements Runnable {
@Inject @Config("sheetLockTimeout") Duration timeout; @Inject @Config("sheetLockTimeout") Duration timeout;
@Inject @Config("sheetRegistrarId") Optional<String> idConfig; @Inject @Config("sheetRegistrarId") Optional<String> idConfig;
@Inject @Parameter("id") Optional<String> idParam; @Inject @Parameter("id") Optional<String> idParam;
@Inject LockHandler lockHandler;
@Inject SyncRegistrarsSheetAction() {} @Inject SyncRegistrarsSheetAction() {}
@Override @Override
@ -146,7 +147,7 @@ public class SyncRegistrarsSheetAction implements Runnable {
return null; 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 // 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 // which case it should be safe to not bother. The task queue definition should *not* specify
// max-concurrent-requests for this very reason. // max-concurrent-requests for this very reason.

View file

@ -12,7 +12,7 @@ java_library(
), ),
deps = [ deps = [
"//java/google/registry/request/auth", "//java/google/registry/request/auth",
"//java/google/registry/security", "//java/google/registry/request/lock",
"//java/google/registry/util", "//java/google/registry/util",
"@com_google_appengine_api_1_0_sdk", "@com_google_appengine_api_1_0_sdk",
"@com_google_auto_value", "@com_google_auto_value",

View file

@ -27,6 +27,8 @@ import dagger.Provides;
import google.registry.request.HttpException.BadRequestException; import google.registry.request.HttpException.BadRequestException;
import google.registry.request.HttpException.UnsupportedMediaTypeException; import google.registry.request.HttpException.UnsupportedMediaTypeException;
import google.registry.request.auth.AuthResult; import google.registry.request.auth.AuthResult;
import google.registry.request.lock.LockHandler;
import google.registry.request.lock.LockHandlerPassthrough;
import java.io.IOException; import java.io.IOException;
import java.util.Map; import java.util.Map;
import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest;
@ -122,6 +124,11 @@ public final class RequestModule {
} }
} }
@Provides
static LockHandler provideLockHandler(LockHandlerPassthrough lockHandler) {
return lockHandler;
}
@Provides @Provides
@JsonPayload @JsonPayload
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")

View file

@ -24,12 +24,12 @@ import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when; import static org.mockito.Mockito.when;
import com.google.common.base.Optional; import com.google.common.base.Optional;
import google.registry.model.server.Lock;
import google.registry.testing.AppEngineRule; import google.registry.testing.AppEngineRule;
import google.registry.testing.FakeLockHandler;
import google.registry.testing.FakeResponse; import google.registry.testing.FakeResponse;
import java.util.concurrent.Callable;
import javax.annotation.Nullable; import javax.annotation.Nullable;
import org.joda.time.Duration; import org.joda.time.Duration;
import org.junit.Before;
import org.junit.Rule; import org.junit.Rule;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
@ -48,16 +48,23 @@ public class SyncRegistrarsSheetActionTest {
private final FakeResponse response = new FakeResponse(); private final FakeResponse response = new FakeResponse();
private final SyncRegistrarsSheet syncRegistrarsSheet = mock(SyncRegistrarsSheet.class); private final SyncRegistrarsSheet syncRegistrarsSheet = mock(SyncRegistrarsSheet.class);
private SyncRegistrarsSheetAction action;
private void runAction(@Nullable String idConfig, @Nullable String idParam) { 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.idConfig = Optional.fromNullable(idConfig);
action.idParam = Optional.fromNullable(idParam); action.idParam = Optional.fromNullable(idParam);
action.timeout = Duration.standardHours(1);
action.run(); 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 @Test
public void testPost_withoutParamsOrSystemProperty_dropsTask() throws Exception { public void testPost_withoutParamsOrSystemProperty_dropsTask() throws Exception {
runAction(null, null); runAction(null, null);
@ -96,14 +103,8 @@ public class SyncRegistrarsSheetActionTest {
@Test @Test
public void testPost_failToAquireLock_servletDoesNothingAndReturns() throws Exception { public void testPost_failToAquireLock_servletDoesNothingAndReturns() throws Exception {
String lockName = "Synchronize registrars sheet: foobar"; action.lockHandler = new FakeLockHandler(false);
Lock.executeWithLocks(new Callable<Void>() { runAction(null, "foobar");
@Override
public Void call() throws Exception {
runAction(null, "foobar");
return null;
}
}, null, Duration.standardHours(1), lockName);
assertThat(response.getPayload()).startsWith("LOCKED"); assertThat(response.getPayload()).startsWith("LOCKED");
verifyZeroInteractions(syncRegistrarsSheet); verifyZeroInteractions(syncRegistrarsSheet);
} }

View file

@ -29,6 +29,7 @@ java_library(
"//java/google/registry/monitoring/whitebox", "//java/google/registry/monitoring/whitebox",
"//java/google/registry/pricing", "//java/google/registry/pricing",
"//java/google/registry/request", "//java/google/registry/request",
"//java/google/registry/request/lock",
"//java/google/registry/tmch", "//java/google/registry/tmch",
"//java/google/registry/util", "//java/google/registry/util",
"//java/google/registry/xml", "//java/google/registry/xml",

View file

@ -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<Void> 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;
}
}