diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java index b32950b9f5..88a9794e90 100644 --- a/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -285,8 +285,13 @@ public class AccountsUpdate { private final ExternalIdNotesLoader extIdNotesLoader; private final PersonIdent committerIdent; private final PersonIdent authorIdent; + + // Invoked after reading the account config. private final Runnable afterReadRevision; + // Invoked after updating the account but before committing the changes. + private final Runnable beforeCommit; + private AccountsUpdate( GitRepositoryManager repoManager, GitReferenceUpdated gitRefUpdated, @@ -309,6 +314,7 @@ public class AccountsUpdate { extIdNotesLoader, committerIdent, authorIdent, + Runnables.doNothing(), Runnables.doNothing()); } @@ -324,7 +330,8 @@ public class AccountsUpdate { ExternalIdNotesLoader extIdNotesLoader, PersonIdent committerIdent, PersonIdent authorIdent, - Runnable afterReadRevision) { + Runnable afterReadRevision, + Runnable beforeCommit) { this.repoManager = checkNotNull(repoManager, "repoManager"); this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated"); this.currentUser = currentUser; @@ -336,7 +343,8 @@ public class AccountsUpdate { this.extIdNotesLoader = checkNotNull(extIdNotesLoader, "extIdNotesLoader"); this.committerIdent = checkNotNull(committerIdent, "committerIdent"); this.authorIdent = checkNotNull(authorIdent, "authorIdent"); - this.afterReadRevision = afterReadRevision; + this.afterReadRevision = checkNotNull(afterReadRevision, "afterReadRevision"); + this.beforeCommit = checkNotNull(beforeCommit, "beforeCommit"); } /** @@ -491,6 +499,8 @@ public class AccountsUpdate { } private void commit(Repository allUsersRepo, UpdatedAccount updatedAccount) throws IOException { + beforeCommit.run(); + BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate(); if (updatedAccount.isCreated()) { commitNewAccountConfig( diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index c04f05ec59..01be2b525f 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -46,6 +46,7 @@ import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Iterables; import com.google.common.io.BaseEncoding; import com.google.common.util.concurrent.AtomicLongMap; +import com.google.common.util.concurrent.Runnables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AccountCreator; import com.google.gerrit.acceptance.GerritConfig; @@ -2041,7 +2042,8 @@ public class AccountIT extends AbstractDaemonTest { // Ignore, the successful update of the account is asserted later } } - }); + }, + Runnables.doNothing()); assertThat(doneBgUpdate.get()).isFalse(); AccountInfo accountInfo = gApi.accounts().id(admin.id.get()).get(); assertThat(accountInfo.status).isNull(); @@ -2095,7 +2097,8 @@ public class AccountIT extends AbstractDaemonTest { } catch (IOException | ConfigInvalidException | OrmException e) { // Ignore, the expected exception is asserted later } - }); + }, + Runnables.doNothing()); assertThat(bgCounter.get()).isEqualTo(0); AccountInfo accountInfo = gApi.accounts().id(admin.id.get()).get(); assertThat(accountInfo.status).isNull(); @@ -2118,6 +2121,67 @@ public class AccountIT extends AbstractDaemonTest { assertThat(accountInfo.name).isEqualTo(admin.fullName); } + @Test + public void atomicReadMofifyWrite() throws Exception { + gApi.accounts().id(admin.id.get()).setStatus("A-1"); + + AtomicInteger bgCounterA1 = new AtomicInteger(0); + AtomicInteger bgCounterA2 = new AtomicInteger(0); + PersonIdent ident = serverIdent.get(); + AccountsUpdate update = + new AccountsUpdate( + repoManager, + gitReferenceUpdated, + null, + allUsers, + externalIds, + metaDataUpdateInternalFactory, + new RetryHelper( + cfg, + retryMetrics, + null, + null, + null, + r -> r.withBlockStrategy(noSleepBlockStrategy)), + extIdNotesFactory, + ident, + ident, + Runnables.doNothing(), + () -> { + try { + accountsUpdate.create().update("Set Status", admin.id, u -> u.setStatus("A-2")); + } catch (IOException | ConfigInvalidException | OrmException e) { + // Ignore, the expected exception is asserted later + } + }); + assertThat(bgCounterA1.get()).isEqualTo(0); + assertThat(bgCounterA2.get()).isEqualTo(0); + assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("A-1"); + + Account updatedAccount = + update.update( + "Set Status", + admin.id, + (a, u) -> { + if ("A-1".equals(a.getAccount().getStatus())) { + bgCounterA1.getAndIncrement(); + u.setStatus("B-1"); + } + + if ("A-2".equals(a.getAccount().getStatus())) { + bgCounterA2.getAndIncrement(); + u.setStatus("B-2"); + } + }); + + assertThat(bgCounterA1.get()).isEqualTo(1); + assertThat(bgCounterA2.get()).isEqualTo(1); + + assertThat(updatedAccount.getStatus()).isEqualTo("B-2"); + assertThat(accounts.get(admin.id).getAccount().getStatus()).isEqualTo("B-2"); + assertThat(gApi.accounts().id(admin.id.get()).get().status).isEqualTo("B-2"); + } + @Test public void stalenessChecker() throws Exception { // Newly created account is not stale.