diff --git a/core/src/main/java/google/registry/model/server/Lock.java b/core/src/main/java/google/registry/model/server/Lock.java index d9488be3e..d1aeffb20 100644 --- a/core/src/main/java/google/registry/model/server/Lock.java +++ b/core/src/main/java/google/registry/model/server/Lock.java @@ -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. - * - *

When that request is no longer running (is finished), the lock can be considered implicitly - * released. - * - *

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 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 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); diff --git a/core/src/main/java/google/registry/request/RequestModule.java b/core/src/main/java/google/registry/request/RequestModule.java index 4f76a90c7..ceb915c63 100644 --- a/core/src/main/java/google/registry/request/RequestModule.java +++ b/core/src/main/java/google/registry/request/RequestModule.java @@ -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") diff --git a/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java b/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java index d307a0a69..3b467fcc6 100644 --- a/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java +++ b/core/src/main/java/google/registry/request/lock/LockHandlerImpl.java @@ -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 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 { diff --git a/core/src/test/java/google/registry/model/server/LockTest.java b/core/src/test/java/google/registry/model/server/LockTest.java index 567e3e380..613e1810a 100644 --- a/core/src/test/java/google/registry/model/server/LockTest.java +++ b/core/src/test/java/google/registry/model/server/LockTest.java @@ -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 acquire( String tld, Duration leaseLength, LockState expectedLockState) { Lock.lockMetrics = mock(LockMetrics.class); - Optional lock = Lock.acquire(RESOURCE_NAME, tld, leaseLength, requestStatusChecker, true); + Optional 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"); } } diff --git a/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java b/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java index d00c2d257..2712d7b29 100644 --- a/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java +++ b/core/src/test/java/google/registry/request/lock/LockHandlerImplTest.java @@ -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 diff --git a/core/src/test/java/google/registry/util/RequestStatusCheckerImplTest.java b/core/src/test/java/google/registry/util/RequestStatusCheckerImplTest.java deleted file mode 100644 index ca2bff6fc..000000000 --- a/core/src/test/java/google/registry/util/RequestStatusCheckerImplTest.java +++ /dev/null @@ -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: "); - } -} diff --git a/db/src/main/resources/sql/schema/db-schema.sql.generated b/db/src/main/resources/sql/schema/db-schema.sql.generated index 4c687fe00..8c5e33804 100644 --- a/db/src/main/resources/sql/schema/db-schema.sql.generated +++ b/db/src/main/resources/sql/schema/db-schema.sql.generated @@ -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) ); diff --git a/util/src/main/java/google/registry/util/AppEngineTimeLimiter.java b/util/src/main/java/google/registry/util/AppEngineTimeLimiter.java index 462df1b91..a821cea88 100644 --- a/util/src/main/java/google/registry/util/AppEngineTimeLimiter.java +++ b/util/src/main/java/google/registry/util/AppEngineTimeLimiter.java @@ -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. - * - *

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. - * - *

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. - * - *

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 diff --git a/util/src/main/java/google/registry/util/RequestStatusChecker.java b/util/src/main/java/google/registry/util/RequestStatusChecker.java deleted file mode 100644 index d7e3f0896..000000000 --- a/util/src/main/java/google/registry/util/RequestStatusChecker.java +++ /dev/null @@ -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. - * - *

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); -} - diff --git a/util/src/main/java/google/registry/util/RequestStatusCheckerImpl.java b/util/src/main/java/google/registry/util/RequestStatusCheckerImpl.java deleted file mode 100644 index 4eef4b039..000000000 --- a/util/src/main/java/google/registry/util/RequestStatusCheckerImpl.java +++ /dev/null @@ -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. - * - *

May be safely called multiple times, will always return the same result (within the same - * request). - * - * @see appengine documentation - */ - @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 appengine documentation - */ - @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(); - } -}