Swap all uses of Lock to LockHandler

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=167661348
This commit is contained in:
guyben 2017-09-05 18:23:57 -07:00 committed by jianglai
parent 57bcd6b1eb
commit c3861f6e95
10 changed files with 50 additions and 21 deletions

View file

@ -26,6 +26,7 @@ java_library(
"//java/google/registry/monitoring/metrics", "//java/google/registry/monitoring/metrics",
"//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_appengine_api_1_0_sdk", "@com_google_appengine_api_1_0_sdk",

View file

@ -14,7 +14,6 @@
package google.registry.dns; package google.registry.dns;
import static google.registry.model.server.Lock.executeWithLocks;
import static google.registry.request.Action.Method.POST; import static google.registry.request.Action.Method.POST;
import static google.registry.request.RequestParameters.PARAM_TLD; import static google.registry.request.RequestParameters.PARAM_TLD;
import static google.registry.util.CollectionUtils.nullToEmpty; import static google.registry.util.CollectionUtils.nullToEmpty;
@ -28,6 +27,7 @@ import google.registry.request.Action;
import google.registry.request.HttpException.ServiceUnavailableException; import google.registry.request.HttpException.ServiceUnavailableException;
import google.registry.request.Parameter; import google.registry.request.Parameter;
import google.registry.request.auth.Auth; import google.registry.request.auth.Auth;
import google.registry.request.lock.LockHandler;
import google.registry.util.DomainNameUtils; import google.registry.util.DomainNameUtils;
import google.registry.util.FormattingLogger; import google.registry.util.FormattingLogger;
import java.util.Set; import java.util.Set;
@ -69,6 +69,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable<Void> {
@Inject @Parameter(PARAM_DOMAINS) Set<String> domains; @Inject @Parameter(PARAM_DOMAINS) Set<String> domains;
@Inject @Parameter(PARAM_HOSTS) Set<String> hosts; @Inject @Parameter(PARAM_HOSTS) Set<String> hosts;
@Inject @Parameter(PARAM_TLD) String tld; @Inject @Parameter(PARAM_TLD) String tld;
@Inject LockHandler lockHandler;
@Inject PublishDnsUpdatesAction() {} @Inject PublishDnsUpdatesAction() {}
/** Runs the task. */ /** Runs the task. */
@ -79,7 +80,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable<Void> {
// false. We need to make sure to take note of this error; otherwise, a failed lock might result // false. We need to make sure to take note of this error; otherwise, a failed lock might result
// in the update task being dequeued and dropped. A message will already have been logged // in the update task being dequeued and dropped. A message will already have been logged
// to indicate the problem. // to indicate the problem.
if (!executeWithLocks(this, tld, timeout, lockName)) { if (!lockHandler.executeWithLocks(this, tld, timeout, lockName)) {
throw new ServiceUnavailableException("Lock failure"); throw new ServiceUnavailableException("Lock failure");
} }
} }

View file

@ -16,6 +16,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/tldconfig/idn", "//java/google/registry/tldconfig/idn",
"//java/google/registry/util", "//java/google/registry/util",
"//java/google/registry/xjc", "//java/google/registry/xjc",

View file

@ -20,11 +20,11 @@ import com.googlecode.objectify.VoidWork;
import google.registry.model.common.Cursor; import google.registry.model.common.Cursor;
import google.registry.model.common.Cursor.CursorType; import google.registry.model.common.Cursor.CursorType;
import google.registry.model.registry.Registry; import google.registry.model.registry.Registry;
import google.registry.model.server.Lock;
import google.registry.request.HttpException.NoContentException; import google.registry.request.HttpException.NoContentException;
import google.registry.request.HttpException.ServiceUnavailableException; import google.registry.request.HttpException.ServiceUnavailableException;
import google.registry.request.Parameter; import google.registry.request.Parameter;
import google.registry.request.RequestParameters; import google.registry.request.RequestParameters;
import google.registry.request.lock.LockHandler;
import google.registry.util.Clock; import google.registry.util.Clock;
import google.registry.util.FormattingLogger; import google.registry.util.FormattingLogger;
import java.util.concurrent.Callable; import java.util.concurrent.Callable;
@ -38,7 +38,7 @@ import org.joda.time.Duration;
* <p>This class implements the <i>Locking Rolling Cursor</i> pattern, which solves the problem of * <p>This class implements the <i>Locking Rolling Cursor</i> pattern, which solves the problem of
* how to reliably execute App Engine tasks which can't be made idempotent. * how to reliably execute App Engine tasks which can't be made idempotent.
* *
* <p>{@link Lock} is used to ensure only one task executes at a time for a given * <p>{@link LockHandler} is used to ensure only one task executes at a time for a given
* {@code LockedCursorTask} subclass + TLD combination. This is necessary because App Engine tasks * {@code LockedCursorTask} subclass + TLD combination. This is necessary because App Engine tasks
* might double-execute. Normally tasks solve this by being idempotent, but that's not possible for * might double-execute. Normally tasks solve this by being idempotent, but that's not possible for
* RDE, which writes to a GCS filename with a deterministic name. So Datastore is used to to * RDE, which writes to a GCS filename with a deterministic name. So Datastore is used to to
@ -71,6 +71,7 @@ class EscrowTaskRunner {
@Inject Clock clock; @Inject Clock clock;
@Inject @Parameter(RequestParameters.PARAM_TLD) String tld; @Inject @Parameter(RequestParameters.PARAM_TLD) String tld;
@Inject LockHandler lockHandler;
@Inject EscrowTaskRunner() {} @Inject EscrowTaskRunner() {}
/** /**
@ -109,7 +110,7 @@ class EscrowTaskRunner {
return null; return null;
}}; }};
String lockName = String.format("%s %s", task.getClass().getSimpleName(), registry.getTld()); String lockName = String.format("%s %s", task.getClass().getSimpleName(), registry.getTld());
if (!Lock.executeWithLocks(lockRunner, tld, timeout, lockName)) { if (!lockHandler.executeWithLocks(lockRunner, tld, timeout, lockName)) {
// This will happen if either: a) the task is double-executed; b) the task takes a long time // This will happen if either: a) the task is double-executed; b) the task takes a long time
// to run and the retry task got executed while the first one is still running. In both // to run and the retry task got executed while the first one is still running. In both
// situations the safest thing to do is to just return 503 so the task gets retried later. // situations the safest thing to do is to just return 503 so the task gets retried later.

View file

@ -40,9 +40,9 @@ import google.registry.model.rde.RdeMode;
import google.registry.model.rde.RdeNamingUtils; import google.registry.model.rde.RdeNamingUtils;
import google.registry.model.rde.RdeRevision; import google.registry.model.rde.RdeRevision;
import google.registry.model.registry.Registry; import google.registry.model.registry.Registry;
import google.registry.model.server.Lock;
import google.registry.request.Parameter; import google.registry.request.Parameter;
import google.registry.request.RequestParameters; import google.registry.request.RequestParameters;
import google.registry.request.lock.LockHandler;
import google.registry.tldconfig.idn.IdnTableEnum; import google.registry.tldconfig.idn.IdnTableEnum;
import google.registry.util.FormattingLogger; import google.registry.util.FormattingLogger;
import google.registry.util.TaskEnqueuer; import google.registry.util.TaskEnqueuer;
@ -66,11 +66,12 @@ import org.joda.time.Duration;
/** Reducer for {@link RdeStagingAction}. */ /** Reducer for {@link RdeStagingAction}. */
public final class RdeStagingReducer extends Reducer<PendingDeposit, DepositFragment, Void> { public final class RdeStagingReducer extends Reducer<PendingDeposit, DepositFragment, Void> {
private static final long serialVersionUID = -3366189042770402345L; private static final long serialVersionUID = 60326234579091203L;
private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass(); private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass();
private final TaskEnqueuer taskEnqueuer; private final TaskEnqueuer taskEnqueuer;
private final LockHandler lockHandler;
private final int gcsBufferSize; private final int gcsBufferSize;
private final String bucket; private final String bucket;
private final int ghostrydeBufferSize; private final int ghostrydeBufferSize;
@ -81,6 +82,7 @@ public final class RdeStagingReducer extends Reducer<PendingDeposit, DepositFrag
@Inject @Inject
RdeStagingReducer( RdeStagingReducer(
TaskEnqueuer taskEnqueuer, TaskEnqueuer taskEnqueuer,
LockHandler lockHandler,
@Config("gcsBufferSize") int gcsBufferSize, @Config("gcsBufferSize") int gcsBufferSize,
@Config("rdeBucket") String bucket, @Config("rdeBucket") String bucket,
@Config("rdeGhostrydeBufferSize") int ghostrydeBufferSize, @Config("rdeGhostrydeBufferSize") int ghostrydeBufferSize,
@ -88,6 +90,7 @@ public final class RdeStagingReducer extends Reducer<PendingDeposit, DepositFrag
@KeyModule.Key("rdeStagingEncryptionKey") byte[] stagingKeyBytes, @KeyModule.Key("rdeStagingEncryptionKey") byte[] stagingKeyBytes,
@Parameter(RdeModule.PARAM_LENIENT) boolean lenient) { @Parameter(RdeModule.PARAM_LENIENT) boolean lenient) {
this.taskEnqueuer = taskEnqueuer; this.taskEnqueuer = taskEnqueuer;
this.lockHandler = lockHandler;
this.gcsBufferSize = gcsBufferSize; this.gcsBufferSize = gcsBufferSize;
this.bucket = bucket; this.bucket = bucket;
this.ghostrydeBufferSize = ghostrydeBufferSize; this.ghostrydeBufferSize = ghostrydeBufferSize;
@ -105,7 +108,7 @@ public final class RdeStagingReducer extends Reducer<PendingDeposit, DepositFrag
return null; return null;
}}; }};
String lockName = String.format("RdeStaging %s", key.tld()); String lockName = String.format("RdeStaging %s", key.tld());
if (!Lock.executeWithLocks(lockRunner, null, lockTimeout, lockName)) { if (!lockHandler.executeWithLocks(lockRunner, null, lockTimeout, lockName)) {
logger.warningfmt("Lock in use: %s", lockName); logger.warningfmt("Lock in use: %s", lockName);
} }
} }

View file

@ -30,8 +30,11 @@ import google.registry.dns.writer.DnsWriter;
import google.registry.model.domain.DomainResource; import google.registry.model.domain.DomainResource;
import google.registry.model.ofy.Ofy; import google.registry.model.ofy.Ofy;
import google.registry.model.registry.Registry; import google.registry.model.registry.Registry;
import google.registry.request.HttpException.ServiceUnavailableException;
import google.registry.testing.AppEngineRule; import google.registry.testing.AppEngineRule;
import google.registry.testing.ExceptionRule;
import google.registry.testing.FakeClock; import google.registry.testing.FakeClock;
import google.registry.testing.FakeLockHandler;
import google.registry.testing.InjectRule; import google.registry.testing.InjectRule;
import org.joda.time.DateTime; import org.joda.time.DateTime;
import org.joda.time.Duration; import org.joda.time.Duration;
@ -54,7 +57,11 @@ public class PublishDnsUpdatesActionTest {
@Rule @Rule
public final InjectRule inject = new InjectRule(); public final InjectRule inject = new InjectRule();
@Rule
public final ExceptionRule thrown = new ExceptionRule();
private final FakeClock clock = new FakeClock(DateTime.parse("1971-01-01TZ")); private final FakeClock clock = new FakeClock(DateTime.parse("1971-01-01TZ"));
private final FakeLockHandler lockHandler = new FakeLockHandler(true);
private final DnsWriter dnsWriter = mock(DnsWriter.class); private final DnsWriter dnsWriter = mock(DnsWriter.class);
private final DnsMetrics dnsMetrics = mock(DnsMetrics.class); private final DnsMetrics dnsMetrics = mock(DnsMetrics.class);
private PublishDnsUpdatesAction action; private PublishDnsUpdatesAction action;
@ -82,6 +89,7 @@ public class PublishDnsUpdatesActionTest {
action.dnsWriter = "mock"; action.dnsWriter = "mock";
action.dnsWriterProxy = new DnsWriterProxy(ImmutableMap.of("mock", dnsWriter)); action.dnsWriterProxy = new DnsWriterProxy(ImmutableMap.of("mock", dnsWriter));
action.dnsMetrics = dnsMetrics; action.dnsMetrics = dnsMetrics;
action.lockHandler = lockHandler;
return action; return action;
} }
@ -148,4 +156,14 @@ public class PublishDnsUpdatesActionTest {
verify(dnsMetrics, times(3)).incrementPublishHostRequests("xn--q9jyb4c", Status.REJECTED); verify(dnsMetrics, times(3)).incrementPublishHostRequests("xn--q9jyb4c", Status.REJECTED);
verifyNoMoreInteractions(dnsMetrics); verifyNoMoreInteractions(dnsMetrics);
} }
@Test
public void testLockIsntAvailable() throws Exception {
thrown.expect(ServiceUnavailableException.class, "Lock failure");
action = createAction("xn--q9jyb4c");
action.domains = ImmutableSet.of("example.com", "example2.com");
action.hosts = ImmutableSet.of("ns1.example.com", "ns2.example.com", "ns1.example2.com");
action.lockHandler = new FakeLockHandler(false);
action.run();
}
} }

View file

@ -32,6 +32,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

@ -30,7 +30,9 @@ import google.registry.flows.domain.DomainFlowTmchUtils;
import google.registry.monitoring.whitebox.BigQueryMetricsEnqueuer; import google.registry.monitoring.whitebox.BigQueryMetricsEnqueuer;
import google.registry.monitoring.whitebox.EppMetric; import google.registry.monitoring.whitebox.EppMetric;
import google.registry.request.RequestScope; import google.registry.request.RequestScope;
import google.registry.request.lock.LockHandler;
import google.registry.testing.FakeClock; import google.registry.testing.FakeClock;
import google.registry.testing.FakeLockHandler;
import google.registry.testing.FakeSleeper; import google.registry.testing.FakeSleeper;
import google.registry.tmch.TmchCertificateAuthority; import google.registry.tmch.TmchCertificateAuthority;
import google.registry.tmch.TmchXmlSignature; import google.registry.tmch.TmchXmlSignature;
@ -58,6 +60,7 @@ interface EppTestComponent {
private DomainFlowTmchUtils domainFlowTmchUtils; private DomainFlowTmchUtils domainFlowTmchUtils;
private EppMetric.Builder metricBuilder; private EppMetric.Builder metricBuilder;
private FakeClock clock; private FakeClock clock;
private FakeLockHandler lockHandler;
private ModulesService modulesService; private ModulesService modulesService;
private Sleeper sleeper; private Sleeper sleeper;
@ -85,6 +88,7 @@ interface EppTestComponent {
instance.metricBuilder = eppMetricBuilder; instance.metricBuilder = eppMetricBuilder;
instance.modulesService = mock(ModulesService.class); instance.modulesService = mock(ModulesService.class);
instance.metricsEnqueuer = mock(BigQueryMetricsEnqueuer.class); instance.metricsEnqueuer = mock(BigQueryMetricsEnqueuer.class);
instance.lockHandler = new FakeLockHandler(true);
return instance; return instance;
} }
@ -98,6 +102,11 @@ interface EppTestComponent {
return clock; return clock;
} }
@Provides
LockHandler provideLockHandler() {
return lockHandler;
}
@Provides @Provides
CustomLogicFactory provideCustomLogicFactory() { CustomLogicFactory provideCustomLogicFactory() {
return new TestCustomLogicFactory(); return new TestCustomLogicFactory();

View file

@ -26,14 +26,13 @@ import static org.mockito.Mockito.verify;
import google.registry.model.common.Cursor; import google.registry.model.common.Cursor;
import google.registry.model.common.Cursor.CursorType; import google.registry.model.common.Cursor.CursorType;
import google.registry.model.registry.Registry; import google.registry.model.registry.Registry;
import google.registry.model.server.Lock;
import google.registry.rde.EscrowTaskRunner.EscrowTask; import google.registry.rde.EscrowTaskRunner.EscrowTask;
import google.registry.request.HttpException.NoContentException; import google.registry.request.HttpException.NoContentException;
import google.registry.request.HttpException.ServiceUnavailableException; import google.registry.request.HttpException.ServiceUnavailableException;
import google.registry.testing.AppEngineRule; import google.registry.testing.AppEngineRule;
import google.registry.testing.ExceptionRule; import google.registry.testing.ExceptionRule;
import google.registry.testing.FakeClock; import google.registry.testing.FakeClock;
import java.util.concurrent.Callable; import google.registry.testing.FakeLockHandler;
import org.joda.time.DateTime; import org.joda.time.DateTime;
import org.joda.time.DateTimeZone; import org.joda.time.DateTimeZone;
import org.junit.Before; import org.junit.Before;
@ -68,6 +67,7 @@ public class EscrowTaskRunnerTest {
runner = new EscrowTaskRunner(); runner = new EscrowTaskRunner();
runner.clock = clock; runner.clock = clock;
runner.tld = "lol"; runner.tld = "lol";
runner.lockHandler = new FakeLockHandler(true);
DateTimeZone.setDefault(DateTimeZone.forID("America/New_York")); // Make sure UTC stuff works. DateTimeZone.setDefault(DateTimeZone.forID("America/New_York")); // Make sure UTC stuff works.
} }
@ -112,16 +112,8 @@ public class EscrowTaskRunnerTest {
persistResource( persistResource(
Cursor.create(CursorType.RDE_STAGING, DateTime.parse("2006-06-06TZ"), registry)); Cursor.create(CursorType.RDE_STAGING, DateTime.parse("2006-06-06TZ"), registry));
thrown.expect(ServiceUnavailableException.class, "Lock in use: " + lockName); thrown.expect(ServiceUnavailableException.class, "Lock in use: " + lockName);
Lock.executeWithLocks( runner.lockHandler = new FakeLockHandler(false);
new Callable<Void>() { runner.lockRunAndRollForward(
@Override task, registry, standardSeconds(30), CursorType.RDE_STAGING, standardDays(1));
public Void call() throws Exception {
runner.lockRunAndRollForward(
task, registry, standardSeconds(30), CursorType.RDE_STAGING, standardDays(1));
return null;
}},
"lol",
standardSeconds(30),
lockName);
} }
} }

View file

@ -59,6 +59,7 @@ import google.registry.request.RequestParameters;
import google.registry.testing.ExceptionRule; import google.registry.testing.ExceptionRule;
import google.registry.testing.FakeClock; import google.registry.testing.FakeClock;
import google.registry.testing.FakeKeyringModule; import google.registry.testing.FakeKeyringModule;
import google.registry.testing.FakeLockHandler;
import google.registry.testing.FakeResponse; import google.registry.testing.FakeResponse;
import google.registry.testing.InjectRule; import google.registry.testing.InjectRule;
import google.registry.testing.TaskQueueHelper.TaskMatcher; import google.registry.testing.TaskQueueHelper.TaskMatcher;
@ -137,6 +138,7 @@ public class RdeStagingActionTest extends MapreduceTestCase<RdeStagingAction> {
action.lenient = false; action.lenient = false;
action.reducer = new RdeStagingReducer( action.reducer = new RdeStagingReducer(
new TaskEnqueuer(new Retrier(new SystemSleeper(), 1)), // taskEnqueuer new TaskEnqueuer(new Retrier(new SystemSleeper(), 1)), // taskEnqueuer
new FakeLockHandler(true),
0, // gcsBufferSize 0, // gcsBufferSize
"rde-bucket", // bucket "rde-bucket", // bucket
31337, // ghostrydeBufferSize 31337, // ghostrydeBufferSize