From 8245d2f1c44aaa9a2caaf032f2a2598dfea2d25a Mon Sep 17 00:00:00 2001 From: mcilwain Date: Fri, 22 Jun 2018 10:43:21 -0700 Subject: [PATCH] Make LockHandlerImpl.clock transient SystemClock isn't Serializable (for obvious reasons), whereas LockHandlerImpl is used as a field on some Serializable mapreduce classes. So mark it transient and then re-generate it on first use following de-serialization when it happens to be null. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=201707209 --- .../request/lock/LockHandlerImpl.java | 34 ++++++++++++------- javatests/google/registry/request/lock/BUILD | 1 + .../request/lock/LockHandlerImplTest.java | 4 +-- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/java/google/registry/request/lock/LockHandlerImpl.java b/java/google/registry/request/lock/LockHandlerImpl.java index e6a42f62a..a3b344a33 100644 --- a/java/google/registry/request/lock/LockHandlerImpl.java +++ b/java/google/registry/request/lock/LockHandlerImpl.java @@ -27,6 +27,8 @@ import google.registry.model.server.Lock; import google.registry.util.AppEngineTimeLimiter; import google.registry.util.Clock; import google.registry.util.RequestStatusChecker; +import google.registry.util.SystemClock; +import java.io.Serializable; import java.util.HashSet; import java.util.Optional; import java.util.Set; @@ -40,19 +42,30 @@ import org.joda.time.DateTime; import org.joda.time.Duration; /** Implementation of {@link LockHandler} that uses the datastore lock. */ -public class LockHandlerImpl implements LockHandler { - - private static final long serialVersionUID = 6551645164118637767L; +public class LockHandlerImpl implements LockHandler, Serializable { + private static final long serialVersionUID = 5162259753801400985L; private static final FluentLogger logger = FluentLogger.forEnclosingClass(); /** Fudge factor to make sure we kill threads before a lock actually expires. */ private static final Duration LOCK_TIMEOUT_FUDGE = Duration.standardSeconds(5); - @Inject Clock clock; - @Inject RequestStatusChecker requestStatusChecker; + private final RequestStatusChecker requestStatusChecker; + @Nullable private transient Clock clock; - @Inject public LockHandlerImpl() {} + @Inject + public LockHandlerImpl(RequestStatusChecker requestStatusChecker, Clock clock) { + this.requestStatusChecker = requestStatusChecker; + this.clock = clock; + } + + private synchronized Clock getClock() { + // Re-set the clock on first use after de-serialization + if (clock == null) { + clock = new SystemClock(); + } + return clock; + } /** * Acquire one or more locks and execute a Void {@link Callable}. @@ -70,7 +83,7 @@ public class LockHandlerImpl implements LockHandler { @Nullable String tld, Duration leaseLength, String... lockNames) { - DateTime startTime = clock.nowUtc(); + DateTime startTime = getClock().nowUtc(); String sanitizedTld = Strings.emptyToNull(tld); try { return AppEngineTimeLimiter.create() @@ -87,7 +100,7 @@ public class LockHandlerImpl implements LockHandler { "Execution on locks '%s' for TLD '%s' timed out after %s; started at %s", Joiner.on(", ").join(lockNames), Optional.ofNullable(sanitizedTld).orElse("(null)"), - new Duration(startTime, clock.nowUtc()), + new Duration(startTime, getClock().nowUtc()), startTime), cause); } @@ -113,10 +126,7 @@ public class LockHandlerImpl implements LockHandler { final Set lockNames; LockingCallable( - Callable delegate, - String tld, - Duration leaseLength, - String... lockNames) { + Callable delegate, String tld, Duration leaseLength, String... lockNames) { checkArgument(leaseLength.isLongerThan(LOCK_TIMEOUT_FUDGE)); this.delegate = delegate; this.tld = tld; diff --git a/javatests/google/registry/request/lock/BUILD b/javatests/google/registry/request/lock/BUILD index d79a591ba..528fdd5f7 100644 --- a/javatests/google/registry/request/lock/BUILD +++ b/javatests/google/registry/request/lock/BUILD @@ -14,6 +14,7 @@ java_library( deps = [ "//java/google/registry/model", "//java/google/registry/request/lock", + "//java/google/registry/util", "//javatests/google/registry/testing", "//third_party/java/appengine:appengine-api-testonly", "//third_party/java/appengine:appengine-integration-testing", diff --git a/javatests/google/registry/request/lock/LockHandlerImplTest.java b/javatests/google/registry/request/lock/LockHandlerImplTest.java index 6bde8bf77..d05538e9b 100644 --- a/javatests/google/registry/request/lock/LockHandlerImplTest.java +++ b/javatests/google/registry/request/lock/LockHandlerImplTest.java @@ -23,6 +23,7 @@ import static org.mockito.Mockito.verify; import google.registry.model.server.Lock; import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; +import google.registry.util.RequestStatusCheckerImpl; import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.TimeoutException; @@ -72,7 +73,7 @@ public final class LockHandlerImplTest { } private boolean executeWithLocks(Callable callable, final @Nullable Lock acquiredLock) { - LockHandlerImpl lockHandler = new LockHandlerImpl() { + LockHandlerImpl lockHandler = new LockHandlerImpl(new RequestStatusCheckerImpl(), clock) { private static final long serialVersionUID = 0L; @Override Optional acquire(String resourceName, String tld, Duration leaseLength) { @@ -82,7 +83,6 @@ public final class LockHandlerImplTest { return Optional.ofNullable(acquiredLock); } }; - lockHandler.clock = clock; return lockHandler.executeWithLocks(callable, "tld", ONE_DAY, "resourceName"); }