diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountLockManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountLockManager.java index b09651c0c..50b899b87 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountLockManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountLockManager.java @@ -8,6 +8,7 @@ import com.amazonaws.services.dynamodbv2.ReleaseLockOptions; import com.google.common.annotations.VisibleForTesting; import java.util.ArrayList; import java.util.List; +import java.util.Set; import java.util.UUID; import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; @@ -55,7 +56,7 @@ public class AccountLockManager { * * @throws Exception if an exception is thrown by the given {@code task} */ - public V withLock(final List phoneNumberIdentifiers, + public V withLock(final Set phoneNumberIdentifiers, final Callable task, final Executor lockAcquisitionExecutor) throws Exception { @@ -106,7 +107,7 @@ public class AccountLockManager { * @return a future that completes normally when the given task has executed successfully and all locks have been * released; the returned future may fail with an {@link InterruptedException} if interrupted while acquiring a lock */ - public CompletableFuture withLockAsync(final List phoneNumberIdentifiers, + public CompletableFuture withLockAsync(final Set phoneNumberIdentifiers, final Supplier> taskSupplier, final Executor executor) { if (phoneNumberIdentifiers.isEmpty()) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index 37cb3299f..1d64ceb4a 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -277,7 +277,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen return createTimer.record(() -> { try { - return accountLockManager.withLock(List.of(pni), + return accountLockManager.withLock(Set.of(pni), () -> create(number, pni, accountAttributes, accountBadges, aciIdentityKey, pniIdentityKey, primaryDeviceSpec, userAgent), accountLockExecutor); } catch (final Exception e) { if (e instanceof RuntimeException runtimeException) { @@ -416,7 +416,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen } public CompletableFuture> addDevice(final Account account, final DeviceSpec deviceSpec, final String linkDeviceToken) { - return accountLockManager.withLockAsync(List.of(account.getPhoneNumberIdentifier()), + return accountLockManager.withLockAsync(Set.of(account.getPhoneNumberIdentifier()), () -> addDevice(account.getIdentifier(IdentityType.ACI), deviceSpec, linkDeviceToken, MAX_UPDATE_ATTEMPTS), accountLockExecutor); } @@ -610,7 +610,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen throw new IllegalArgumentException("Cannot remove primary device"); } - return accountLockManager.withLockAsync(List.of(account.getPhoneNumberIdentifier()), + return accountLockManager.withLockAsync(Set.of(account.getPhoneNumberIdentifier()), () -> removeDevice(account.getIdentifier(IdentityType.ACI), deviceId, MAX_UPDATE_ATTEMPTS), accountLockExecutor); } @@ -671,7 +671,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen } try { - return accountLockManager.withLock(List.of(account.getPhoneNumberIdentifier(), targetPhoneNumberIdentifier), + return accountLockManager.withLock(Set.of(account.getPhoneNumberIdentifier(), targetPhoneNumberIdentifier), () -> changeNumber(account, targetNumber, targetPhoneNumberIdentifier, pniIdentityKey, pniSignedPreKeys, pniPqLastResortPreKeys, pniRegistrationIds), accountLockExecutor); } catch (final Exception e) { if (e instanceof MismatchedDevicesException mismatchedDevicesException) { @@ -749,7 +749,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen final Map pniRegistrationIds) throws MismatchedDevicesException { try { - return accountLockManager.withLock(List.of(account.getIdentifier(IdentityType.PNI)), () -> { + return accountLockManager.withLock(Set.of(account.getIdentifier(IdentityType.PNI)), () -> { validateDevices(account, pniSignedPreKeys, pniPqLastResortPreKeys, pniRegistrationIds); final UUID aci = account.getIdentifier(IdentityType.ACI); @@ -1263,7 +1263,7 @@ public class AccountsManager extends RedisPubSubAdapter implemen public CompletableFuture delete(final Account account, final DeletionReason deletionReason) { final Timer.Sample sample = Timer.start(); - return accountLockManager.withLockAsync(List.of(account.getPhoneNumberIdentifier()), () -> delete(account), + return accountLockManager.withLockAsync(Set.of(account.getPhoneNumberIdentifier()), () -> delete(account), accountLockExecutor) .whenComplete((ignored, throwable) -> { sample.stop(deleteTimer); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ClientPublicKeysManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ClientPublicKeysManager.java index 41a6b627d..478f28597 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ClientPublicKeysManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ClientPublicKeysManager.java @@ -2,6 +2,7 @@ package org.whispersystems.textsecuregcm.storage; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; @@ -41,7 +42,7 @@ public class ClientPublicKeysManager { * @return a future that completes when the given key has been stored */ public CompletableFuture setPublicKey(final Account account, final byte deviceId, final ECPublicKey publicKey) { - return accountLockManager.withLockAsync(List.of(account.getPhoneNumberIdentifier()), + return accountLockManager.withLockAsync(Set.of(account.getPhoneNumberIdentifier()), () -> clientPublicKeys.setPublicKey(account.getIdentifier(IdentityType.ACI), deviceId, publicKey), accountLockExecutor); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountLockManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountLockManagerTest.java index 3156e24f7..947ee2706 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountLockManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountLockManagerTest.java @@ -10,7 +10,7 @@ import static org.mockito.Mockito.verify; import com.amazonaws.services.dynamodbv2.AmazonDynamoDBLockClient; import com.amazonaws.services.dynamodbv2.ReleaseLockOptions; import java.util.Collections; -import java.util.List; +import java.util.Set; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutorService; @@ -48,7 +48,7 @@ class AccountLockManagerTest { @Test void withLock() throws Exception { - accountLockManager.withLock(List.of(FIRST_PNI, SECOND_PNI), () -> null, executor); + accountLockManager.withLock(Set.of(FIRST_PNI, SECOND_PNI), () -> null, executor); verify(lockClient, times(2)).acquireLock(any()); verify(lockClient, times(2)).releaseLock(any(ReleaseLockOptions.class)); @@ -56,7 +56,7 @@ class AccountLockManagerTest { @Test void withLockTaskThrowsException() throws InterruptedException { - assertThrows(RuntimeException.class, () -> accountLockManager.withLock(List.of(FIRST_PNI, SECOND_PNI), () -> { + assertThrows(RuntimeException.class, () -> accountLockManager.withLock(Set.of(FIRST_PNI, SECOND_PNI), () -> { throw new RuntimeException(); }, executor)); @@ -68,7 +68,7 @@ class AccountLockManagerTest { void withLockEmptyList() { final Runnable task = mock(Runnable.class); - assertThrows(IllegalArgumentException.class, () -> accountLockManager.withLock(Collections.emptyList(), () -> null, + assertThrows(IllegalArgumentException.class, () -> accountLockManager.withLock(Collections.emptySet(), () -> null, executor)); verify(task, never()).run(); } @@ -76,7 +76,7 @@ class AccountLockManagerTest { @Test void withLockAsync() throws InterruptedException { accountLockManager.withLockAsync( - List.of(FIRST_PNI, SECOND_PNI), () -> CompletableFuture.completedFuture(null), executor).join(); + Set.of(FIRST_PNI, SECOND_PNI), () -> CompletableFuture.completedFuture(null), executor).join(); verify(lockClient, times(2)).acquireLock(any()); verify(lockClient, times(2)).releaseLock(any(ReleaseLockOptions.class)); @@ -86,7 +86,7 @@ class AccountLockManagerTest { void withLockAsyncTaskThrowsException() throws InterruptedException { assertThrows(RuntimeException.class, () -> accountLockManager.withLockAsync( - List.of(FIRST_PNI, SECOND_PNI), () -> CompletableFuture.failedFuture(new RuntimeException()), executor) + Set.of(FIRST_PNI, SECOND_PNI), () -> CompletableFuture.failedFuture(new RuntimeException()), executor) .join()); verify(lockClient, times(2)).acquireLock(any()); @@ -98,7 +98,7 @@ class AccountLockManagerTest { final Runnable task = mock(Runnable.class); assertThrows(IllegalArgumentException.class, - () -> accountLockManager.withLockAsync(Collections.emptyList(), () -> CompletableFuture.completedFuture(null), + () -> accountLockManager.withLockAsync(Collections.emptySet(), () -> CompletableFuture.completedFuture(null), executor)); verify(task, never()).run(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java index f5382e990..25af45fed 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -10,8 +10,8 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anySet; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doAnswer; @@ -112,9 +112,9 @@ class AccountsManagerConcurrentModificationIntegrationTest { doAnswer(invocation -> { final Callable task = invocation.getArgument(1); return task.call(); - }).when(accountLockManager).withLock(anyList(), any(), any()); + }).when(accountLockManager).withLock(anySet(), any(), any()); - when(accountLockManager.withLockAsync(anyList(), any(), any())).thenAnswer(invocation -> { + when(accountLockManager.withLockAsync(anySet(), any(), any())).thenAnswer(invocation -> { final Supplier> taskSupplier = invocation.getArgument(1); taskSupplier.get().join(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java index 9c4e1a4af..7e664cb4e 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -16,8 +16,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyByte; -import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anySet; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.notNull; @@ -218,9 +218,9 @@ class AccountsManagerTest { doAnswer(invocation -> { final Callable task = invocation.getArgument(1); return task.call(); - }).when(accountLockManager).withLock(anyList(), any(), any()); + }).when(accountLockManager).withLock(anySet(), any(), any()); - when(accountLockManager.withLockAsync(anyList(), any(), any())).thenAnswer(invocation -> { + when(accountLockManager.withLockAsync(anySet(), any(), any())).thenAnswer(invocation -> { final Supplier> taskSupplier = invocation.getArgument(1); return taskSupplier.get(); }); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java index 2a78392e5..a872f21f9 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerUsernameIntegrationTest.java @@ -11,7 +11,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.anyList; +import static org.mockito.ArgumentMatchers.anySet; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -131,9 +131,9 @@ class AccountsManagerUsernameIntegrationTest { doAnswer(invocation -> { final Callable task = invocation.getArgument(1); return task.call(); - }).when(accountLockManager).withLock(anyList(), any(), any()); + }).when(accountLockManager).withLock(anySet(), any(), any()); - when(accountLockManager.withLockAsync(anyList(), any(), any())).thenAnswer(invocation -> { + when(accountLockManager.withLockAsync(anySet(), any(), any())).thenAnswer(invocation -> { final Supplier> taskSupplier = invocation.getArgument(1); taskSupplier.get().join();