Remove app engine deps from Lock (#1910)

This commit is contained in:
Pavlo Tkach 2023-03-09 10:47:48 -05:00 committed by GitHub
parent 09d19d996d
commit 7146260a08
10 changed files with 15 additions and 350 deletions

View file

@ -25,8 +25,6 @@ import com.google.common.base.Strings;
import com.google.common.flogger.FluentLogger;
import google.registry.model.ImmutableObject;
import google.registry.persistence.VKey;
import google.registry.util.RequestStatusChecker;
import google.registry.util.RequestStatusCheckerImpl;
import java.io.Serializable;
import java.util.Optional;
import java.util.function.Supplier;
@ -70,8 +68,7 @@ public class Lock extends ImmutableObject implements Serializable {
enum LockState {
IN_USE,
FREE,
TIMED_OUT,
OWNER_DIED
TIMED_OUT
}
@VisibleForTesting static LockMetrics lockMetrics = new LockMetrics();
@ -79,17 +76,6 @@ public class Lock extends ImmutableObject implements Serializable {
/** The name of the locked resource. */
@Transient @Id String lockId;
/**
* Unique log ID of the request that owns this lock.
*
* <p>When that request is no longer running (is finished), the lock can be considered implicitly
* released.
*
* <p>See {@link RequestStatusCheckerImpl#getLogId} for details about how it's created in
* practice.
*/
@Column String requestLogId;
/** When the lock can be considered implicitly released. */
@Column(nullable = false)
DateTime expirationTime;
@ -124,7 +110,6 @@ public class Lock extends ImmutableObject implements Serializable {
private static Lock create(
String resourceName,
String scope,
String requestLogId,
DateTime acquiredTime,
Duration leaseLength) {
checkArgument(!Strings.isNullOrEmpty(resourceName), "resourceName cannot be null or empty");
@ -132,7 +117,6 @@ public class Lock extends ImmutableObject implements Serializable {
// Add the scope to the Lock's id so that it is unique for locks acquiring the same resource
// across different TLDs.
instance.lockId = makeLockId(resourceName, scope);
instance.requestLogId = requestLogId;
instance.expirationTime = acquiredTime.plus(leaseLength);
instance.acquiredTime = acquiredTime;
instance.resourceName = resourceName;
@ -172,18 +156,13 @@ public class Lock extends ImmutableObject implements Serializable {
switch (acquireResult.lockState()) {
case IN_USE:
logger.atInfo().log(
"Existing lock by request %s is still valid now %s (until %s) lock: %s",
lock.requestLogId, now, lock.expirationTime, lock.lockId);
"Existing lock by request is still valid now %s (until %s) lock: %s",
now, lock.expirationTime, lock.lockId);
break;
case TIMED_OUT:
logger.atInfo().log(
"Existing lock by request %s is timed out now %s (was valid until %s) lock: %s",
lock.requestLogId, now, lock.expirationTime, lock.lockId);
break;
case OWNER_DIED:
logger.atInfo().log(
"Existing lock is valid now %s (until %s), but owner (%s) isn't running lock: %s",
now, lock.expirationTime, lock.requestLogId, lock.lockId);
"Existing lock by request is timed out now %s (was valid until %s) lock: %s",
now, lock.expirationTime, lock.lockId);
break;
case FREE:
// There was no existing lock
@ -203,11 +182,7 @@ public class Lock extends ImmutableObject implements Serializable {
/** Try to acquire a lock. Returns absent if it can't be acquired. */
public static Optional<Lock> acquire(
String resourceName,
@Nullable String tld,
Duration leaseLength,
RequestStatusChecker requestStatusChecker,
boolean checkThreadRunning) {
String resourceName, @Nullable String tld, Duration leaseLength) {
String scope = tld != null ? tld : GLOBAL;
Supplier<AcquireResult> lockAcquirer =
() -> {
@ -219,22 +194,19 @@ public class Lock extends ImmutableObject implements Serializable {
.orElse(null);
if (lock != null) {
logger.atInfo().log(
"Loaded existing lock: %s for request: %s", lock.lockId, lock.requestLogId);
"Loaded existing lock: %s for resource: %s", lock.lockId, lock.resourceName);
}
LockState lockState;
if (lock == null) {
lockState = LockState.FREE;
} else if (isAtOrAfter(now, lock.expirationTime)) {
lockState = LockState.TIMED_OUT;
} else if (checkThreadRunning && !requestStatusChecker.isRunning(lock.requestLogId)) {
lockState = LockState.OWNER_DIED;
} else {
lockState = LockState.IN_USE;
return AcquireResult.create(now, lock, null, lockState);
}
Lock newLock =
create(resourceName, scope, requestStatusChecker.getLogId(), now, leaseLength);
Lock newLock = create(resourceName, scope, now, leaseLength);
tm().put(newLock);
return AcquireResult.create(now, lock, newLock, lockState);

View file

@ -39,8 +39,6 @@ import google.registry.request.HttpException.UnsupportedMediaTypeException;
import google.registry.request.auth.AuthResult;
import google.registry.request.lock.LockHandler;
import google.registry.request.lock.LockHandlerImpl;
import google.registry.util.RequestStatusChecker;
import google.registry.util.RequestStatusCheckerImpl;
import java.io.IOException;
import java.util.Map;
import java.util.Optional;
@ -192,18 +190,6 @@ public final class RequestModule {
return lockHandler;
}
@Provides
static RequestStatusChecker provideRequestStatusChecker(
RequestStatusCheckerImpl requestStatusChecker) {
return requestStatusChecker;
}
@Provides
@RequestLogId
static String provideRequestLogId(RequestStatusChecker requestStatusChecker) {
return requestStatusChecker.getLogId();
}
@Provides
@JsonPayload
@SuppressWarnings("unchecked")

View file

@ -26,7 +26,6 @@ import com.google.common.util.concurrent.UncheckedExecutionException;
import google.registry.model.server.Lock;
import google.registry.util.AppEngineTimeLimiter;
import google.registry.util.Clock;
import google.registry.util.RequestStatusChecker;
import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
@ -48,12 +47,10 @@ public class LockHandlerImpl implements LockHandler {
/** Fudge factor to make sure we kill threads before a lock actually expires. */
private static final Duration LOCK_TIMEOUT_FUDGE = Duration.standardSeconds(5);
private final RequestStatusChecker requestStatusChecker;
private final Clock clock;
@Inject
public LockHandlerImpl(RequestStatusChecker requestStatusChecker, Clock clock) {
this.requestStatusChecker = requestStatusChecker;
public LockHandlerImpl(Clock clock) {
this.clock = clock;
}
@ -114,7 +111,7 @@ public class LockHandlerImpl implements LockHandler {
/** Allows injection of mock Lock in tests. */
@VisibleForTesting
Optional<Lock> acquire(String lockName, @Nullable String tld, Duration leaseLength) {
return Lock.acquire(lockName, tld, leaseLength, requestStatusChecker, true);
return Lock.acquire(lockName, tld, leaseLength);
}
private interface LockAcquirer {

View file

@ -18,17 +18,14 @@ import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static google.registry.model.server.Lock.LockState.FREE;
import static google.registry.model.server.Lock.LockState.IN_USE;
import static google.registry.model.server.Lock.LockState.OWNER_DIED;
import static google.registry.model.server.Lock.LockState.TIMED_OUT;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
import google.registry.model.EntityTestCase;
import google.registry.model.server.Lock.LockState;
import google.registry.util.RequestStatusChecker;
import java.util.Optional;
import org.joda.time.Duration;
import org.junit.jupiter.api.AfterEach;
@ -41,7 +38,6 @@ public class LockTest extends EntityTestCase {
private static final String RESOURCE_NAME = "foo";
private static final Duration ONE_DAY = Duration.standardDays(1);
private static final Duration TWO_MILLIS = Duration.millis(2);
private static final RequestStatusChecker requestStatusChecker = mock(RequestStatusChecker.class);
private LockMetrics origLockMetrics;
@ -52,7 +48,7 @@ public class LockTest extends EntityTestCase {
private static Optional<Lock> acquire(
String tld, Duration leaseLength, LockState expectedLockState) {
Lock.lockMetrics = mock(LockMetrics.class);
Optional<Lock> lock = Lock.acquire(RESOURCE_NAME, tld, leaseLength, requestStatusChecker, true);
Optional<Lock> lock = Lock.acquire(RESOURCE_NAME, tld, leaseLength);
verify(Lock.lockMetrics).recordAcquire(RESOURCE_NAME, tld, expectedLockState);
verifyNoMoreInteractions(Lock.lockMetrics);
Lock.lockMetrics = null;
@ -72,8 +68,6 @@ public class LockTest extends EntityTestCase {
void beforeEach() {
origLockMetrics = Lock.lockMetrics;
Lock.lockMetrics = null;
when(requestStatusChecker.getLogId()).thenReturn("current-request-id");
when(requestStatusChecker.isRunning("current-request-id")).thenReturn(true);
}
@AfterEach
@ -111,9 +105,6 @@ public class LockTest extends EntityTestCase {
assertThat(acquire("", ONE_DAY, FREE)).isPresent();
// We can't get it again while request is active
assertThat(acquire("", ONE_DAY, IN_USE)).isEmpty();
// But if request is finished, we can get it.
when(requestStatusChecker.isRunning("current-request-id")).thenReturn(false);
assertThat(acquire("", ONE_DAY, OWNER_DIED)).isPresent();
}
@Test
@ -134,9 +125,7 @@ public class LockTest extends EntityTestCase {
@Test
void testFailure_emptyResourceName() {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> Lock.acquire("", "", TWO_MILLIS, requestStatusChecker, true));
assertThrows(IllegalArgumentException.class, () -> Lock.acquire("", "", TWO_MILLIS));
assertThat(thrown).hasMessageThat().contains("resourceName cannot be null or empty");
}
}

View file

@ -23,7 +23,6 @@ import static org.mockito.Mockito.verify;
import google.registry.model.server.Lock;
import google.registry.testing.FakeClock;
import google.registry.testing.UserServiceExtension;
import google.registry.util.RequestStatusCheckerImpl;
import java.util.Optional;
import java.util.concurrent.Callable;
import java.util.concurrent.TimeoutException;
@ -134,7 +133,7 @@ final class LockHandlerImplTest {
}
private LockHandler createTestLockHandler(@Nullable Lock acquiredLock) {
return new LockHandlerImpl(new RequestStatusCheckerImpl(), clock) {
return new LockHandlerImpl(clock) {
private static final long serialVersionUID = 0L;
@Override

View file

@ -1,129 +0,0 @@
// 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.util;
import static com.google.common.truth.Truth.assertThat;
import static google.registry.testing.LogsSubject.assertAboutLogs;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import com.google.appengine.api.log.LogQuery;
import com.google.appengine.api.log.LogService;
import com.google.appengine.api.log.RequestLogs;
import com.google.apphosting.api.ApiProxy;
import com.google.common.collect.ImmutableList;
import com.google.common.testing.TestLogHandler;
import google.registry.testing.UserServiceExtension;
import java.util.logging.Level;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
/** Unit tests for {@link RequestStatusCheckerImpl}. */
final class RequestStatusCheckerImplTest {
private static final TestLogHandler logHandler = new TestLogHandler();
private static final RequestStatusChecker requestStatusChecker = new RequestStatusCheckerImpl();
/**
* Matcher for the expected LogQuery in {@link RequestStatusCheckerImpl#isRunning}.
*
* Because LogQuery doesn't have a .equals function, we have to create an actual matcher to make
* sure we have the right argument in our mocks.
*/
private static LogQuery expectedLogQuery(final String requestLogId) {
return argThat(
object -> {
assertThat(object).isInstanceOf(LogQuery.class);
assertThat(object.getRequestIds()).containsExactly(requestLogId);
assertThat(object.getIncludeAppLogs()).isFalse();
assertThat(object.getIncludeIncomplete()).isTrue();
return true;
});
}
// We do not actually need to set up user service, rather, we just need this extension to set up
// App Engine environment so the status checker can make an App Engine API call.
@RegisterExtension UserServiceExtension userService = new UserServiceExtension("");
@BeforeEach
void beforeEach() {
JdkLoggerConfig.getConfig(RequestStatusCheckerImpl.class).addHandler(logHandler);
RequestStatusCheckerImpl.logService = mock(LogService.class);
}
@AfterEach
void afterEach() {
JdkLoggerConfig.getConfig(RequestStatusCheckerImpl.class).removeHandler(logHandler);
}
// If a logId is unrecognized, it could be that the log hasn't been uploaded yet - so we assume
// it's a request that has just started running recently.
@Test
void testIsRunning_unrecognized() {
when(RequestStatusCheckerImpl.logService.fetch(expectedLogQuery("12345678")))
.thenReturn(ImmutableList.of());
assertThat(requestStatusChecker.isRunning("12345678")).isTrue();
assertAboutLogs()
.that(logHandler)
.hasLogAtLevelWithMessage(Level.INFO, "Queried an unrecognized requestLogId");
}
@Test
void testIsRunning_notFinished() {
RequestLogs requestLogs = new RequestLogs();
requestLogs.setFinished(false);
when(RequestStatusCheckerImpl.logService.fetch(expectedLogQuery("12345678")))
.thenReturn(ImmutableList.of(requestLogs));
assertThat(requestStatusChecker.isRunning("12345678")).isTrue();
assertAboutLogs()
.that(logHandler)
.hasLogAtLevelWithMessage(Level.INFO, "isFinished: false");
}
@Test
void testIsRunning_finished() {
RequestLogs requestLogs = new RequestLogs();
requestLogs.setFinished(true);
when(RequestStatusCheckerImpl.logService.fetch(expectedLogQuery("12345678")))
.thenReturn(ImmutableList.of(requestLogs));
assertThat(requestStatusChecker.isRunning("12345678")).isFalse();
assertAboutLogs()
.that(logHandler)
.hasLogAtLevelWithMessage(Level.INFO, "isFinished: true");
}
@Test
void testGetLogId_returnsRequestLogId() {
String expectedLogId = ApiProxy.getCurrentEnvironment().getAttributes().get(
"com.google.appengine.runtime.request_log_id").toString();
assertThat(requestStatusChecker.getLogId()).isEqualTo(expectedLogId);
}
@Test
void testGetLogId_createsLog() {
requestStatusChecker.getLogId();
assertAboutLogs()
.that(logHandler)
.hasLogAtLevelWithMessage(Level.INFO, "Current requestLogId: ");
}
}

View file

@ -488,7 +488,6 @@
scope text not null,
acquired_time timestamptz not null,
expiration_time timestamptz not null,
request_log_id text,
primary key (resource_name, scope)
);

View file

@ -14,39 +14,20 @@
package google.registry.util;
import static com.google.appengine.api.ThreadManager.currentRequestThreadFactory;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.SimpleTimeLimiter;
import com.google.common.util.concurrent.TimeLimiter;
import java.util.List;
import java.util.concurrent.AbstractExecutorService;
import java.util.concurrent.TimeUnit;
/**
* A factory for {@link TimeLimiter} instances that use request threads, which carry the namespace
* and live only as long as the request that spawned them.
*
* <p>It is safe to reuse instances of this class, but there is no benefit in doing so over creating
* a fresh instance each time.
*/
public class AppEngineTimeLimiter {
/**
* An {@code ExecutorService} that uses a new thread for every task.
*
* <p>We need to use fresh threads for each request so that we can use App Engine's request
* threads. If we cached these threads in a thread pool (and if we were executing on a backend,
* where there is no time limit on requests) the caching would cause the thread to keep the task
* that opened it alive even after returning an http response, and would also cause the namespace
* that the original thread was created in to leak out to later reuses of the thread.
*
* <p>Since there are no cached resources, this class doesn't have to support being shutdown.
*/
private static class NewRequestThreadExecutorService extends AbstractExecutorService {
@Override
public void execute(Runnable command) {
currentRequestThreadFactory().newThread(command).start();
MoreExecutors.platformThreadFactory().newThread(command).start();
}
@Override

View file

@ -1,34 +0,0 @@
// 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.util;
import java.io.Serializable;
/** Used to query whether requests are still running. */
public interface RequestStatusChecker extends Serializable {
/**
* Returns the unique log identifier of the current request.
*
* <p>Multiple calls must return the same value during the same Request.
*/
String getLogId();
/**
* Returns true if the given request is currently running.
*/
boolean isRunning(String requestLogId);
}

View file

@ -1,95 +0,0 @@
// 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.util;
import com.google.appengine.api.log.LogQuery;
import com.google.appengine.api.log.LogService;
import com.google.appengine.api.log.LogServiceFactory;
import com.google.appengine.api.log.RequestLogs;
import com.google.apphosting.api.ApiProxy;
import com.google.apphosting.api.ApiProxy.Environment;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import java.util.Collections;
import javax.inject.Inject;
/** Implementation of the {@link RequestStatusChecker} interface. */
public class RequestStatusCheckerImpl implements RequestStatusChecker {
private static final long serialVersionUID = -8161977032130865437L;
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@VisibleForTesting
static LogService logService = LogServiceFactory.getLogService();
/**
* The key to {@link Environment#getAttributes}'s request_log_id value.
*/
private static final String REQUEST_LOG_ID_KEY = "com.google.appengine.runtime.request_log_id";
@Inject public RequestStatusCheckerImpl() {}
/**
* Returns the unique log identifier of the current request.
*
* <p>May be safely called multiple times, will always return the same result (within the same
* request).
*
* @see <a href="https://cloud.google.com/appengine/docs/standard/java/how-requests-are-handled#request-ids">appengine documentation</a>
*/
@Override
public String getLogId() {
String requestLogId =
ApiProxy.getCurrentEnvironment().getAttributes().get(REQUEST_LOG_ID_KEY).toString();
logger.atInfo().log("Current requestLogId: %s.", requestLogId);
// We want to make sure there actually is a log to query for this request, even if the request
// dies right after this call.
//
// flushLogs() is synchronous, so once the function returns, no matter what happens next, the
// returned requestLogId will point to existing logs.
ApiProxy.flushLogs();
return requestLogId;
}
/**
* Returns true if the given request is currently running.
*
* @see <a href="https://cloud.google.com/appengine/docs/standard/java/javadoc/com/google/appengine/api/log/LogQuery">appengine documentation</a>
*/
@Override
public boolean isRunning(String requestLogId) {
RequestLogs requestLogs =
Iterables.getOnlyElement(
logService.fetch(
LogQuery.Builder
.withRequestIds(Collections.singletonList(requestLogId))
.includeAppLogs(false)
.includeIncomplete(true)),
null);
// requestLogs will be null if that requestLogId isn't found at all, which can happen if the
// request is too new (it can take several seconds until the logs are available for "fetch").
// So we have to assume it's "running" in that case.
if (requestLogs == null) {
logger.atInfo().log(
"Queried an unrecognized requestLogId %s - assume it's running.", requestLogId);
return true;
}
logger.atInfo().log(
"Found logs for requestLogId %s - isFinished: %b", requestLogId, requestLogs.isFinished());
return !requestLogs.isFinished();
}
}