opendev/gerrit
Code Issues Proposed changes

Test that read-modify-write is atomic on account updates

Browse Source 
When updating an account the account updater gets the current
AccountState provided so that based on this account state it can be
decided which update should be done. If a concurrent request updates the
account after the account updater is invoked it is expected that the
account update fails with LockFailure because the HEAD of the user
branch was updated and no longer matches the revision from which the
account was read. The LockFailure then causes a retry of the account
update and the account updater is invoked again, this time with a new
AccountState that includes the modifications that were done by the
concurrent update. This way the full read-modify-write sequence is
atomic on account updates.

Change-Id: Ia570cd2e99557bf942c1571eae3ef72dd07663dd
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-01-11 15:47:44 +01:00
parent dc22ac98ba
commit a0bc739b29
2 changed files with 78 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -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(

68
javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

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