opendev/gerrit
Code Issues Proposed changes

Reread ChangeData in email constructors

Browse Source 
Email processing is often kicked off into a background thread, so it
is not safe to use the non-threadsafe accessors of Change. Rework the
ChangeEmail hierarchy so a new ChangeData is created in each
constructor, which ensures data is reread in a background thread. This
is a little slow, particularly as not all emails are sent async, but
is the best way we have of guaranteeing safety. It is also a little
ugly as the destination project/branch need to be passed to a super
constructor, and we can't have a statement creating this new
ChangeData prior to the super call, so the ChangeData needs to be
explicitly created in each leaf type.

Change-Id: Ibc549791e5cb2a23fbc4d7acca96589782757296
This commit is contained in:
Dave Borowitz
2015-04-16 13:09:12 -07:00
parent 5f747fecea
commit b8a120b338
23 changed files with 84 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
View File

@@ -317,7 +317,7 @@ public class ChangeUtil {
ins.setMessage(cmsg).insert();
try {
RevertedSender cm = revertedSenderFactory.create(change);
RevertedSender cm = revertedSenderFactory.create(change.getId());
cm.setFrom(user().getAccountId());
cm.setChangeMessage(cmsg);
cm.send();

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java
View File

@@ -126,7 +126,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
CheckedFuture<?, IOException> indexFuture =
indexer.indexAsync(change.getId());
try {
ReplyToChangeSender cm = abandonedSenderFactory.create(change);
ReplyToChangeSender cm = abandonedSenderFactory.create(change.getId());
cm.setFrom(caller.getAccountId());
cm.setChangeMessage(message);
cm.send();

2
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -244,7 +244,7 @@ public class ChangeInserter {
public void run() {
try {
CreateChangeSender cm =
createChangeSenderFactory.create(change);
createChangeSenderFactory.create(change.getId());
cm.setFrom(change.getOwner());
cm.setPatchSet(patchSet, patchSetInfo);
cm.addReviewers(reviewers);

2
gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java
View File

@@ -129,7 +129,7 @@ public class EmailReviewComments implements Runnable, RequestContext {
}
});
CommentSender cm = commentSenderFactory.create(notify, change);
CommentSender cm = commentSenderFactory.create(notify, change.getId());
cm.setFrom(authorId);
cm.setPatchSet(patchSet, patchSetInfoFactory.get(change, patchSet));
cm.setChangeMessage(message);

2
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -293,7 +293,7 @@ public class PatchSetInserter {
try {
PatchSetInfo info = patchSetInfoFactory.get(commit, patchSet.getId());
ReplacePatchSetSender cm =
replacePatchSetFactory.create(updatedChange);
replacePatchSetFactory.create(c.getId());
cm.setFrom(user.getAccountId());
cm.setPatchSet(patchSet, info);
cm.setChangeMessage(changeMessage);

2
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
View File

@@ -283,7 +283,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
}
if (!toMail.isEmpty()) {
try {
AddReviewerSender cm = addReviewerSenderFactory.create(change);
AddReviewerSender cm = addReviewerSenderFactory.create(change.getId());
cm.setFrom(identifiedUser.getAccountId());
cm.addReviewers(toMail);
cm.send();

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java
View File

@@ -125,7 +125,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
CheckedFuture<?, IOException> f = indexer.indexAsync(change.getId());
try {
ReplyToChangeSender cm = restoredSenderFactory.create(change);
ReplyToChangeSender cm = restoredSenderFactory.create(change.getId());
cm.setFrom(caller.getAccountId());
cm.setChangeMessage(message);
cm.send();

4
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -1045,7 +1045,7 @@ public class MergeOp {
}
try {
MergedSender cm = mergedSenderFactory.create(changeControl(c));
MergedSender cm = mergedSenderFactory.create(c.getId());
if (from != null) {
cm.setFrom(from.getAccountId());
}
@@ -1212,7 +1212,7 @@ public class MergeOp {
}
try {
MergeFailSender cm = mergeFailSenderFactory.create(c);
MergeFailSender cm = mergeFailSenderFactory.create(c.getId());
if (from != null) {
cm.setFrom(from.getAccountId());
}

5
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -2214,7 +2214,7 @@ public class ReceiveCommits {
public void run() {
try {
ReplacePatchSetSender cm =
replacePatchSetFactory.create(change);
replacePatchSetFactory.create(change.getId());
cm.setFrom(me);
cm.setPatchSet(newPatchSet, info);
cm.setChangeMessage(msg);
@@ -2594,12 +2594,13 @@ public class ReceiveCommits {
}
private void sendMergedEmail(final ReplaceRequest result) {
final Change.Id id = result.change.getId();
workQueue.getDefaultQueue()
.submit(requestScopePropagator.wrap(new Runnable() {
@Override
public void run() {
try {
final MergedSender cm = mergedSenderFactory.create(result.changeCtl);
final MergedSender cm = mergedSenderFactory.create(id);
cm.setFrom(currentUser.getAccountId());
cm.setPatchSet(result.newPatchSet, result.info);
cm.send();

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -25,12 +26,13 @@ public class AbandonedSender extends ReplyToChangeSender {
public static interface Factory extends
ReplyToChangeSender.Factory<AbandonedSender> {
@Override
AbandonedSender create(Change change);
AbandonedSender create(Change.Id change);
}
@Inject
public AbandonedSender(EmailArguments ea, @Assisted Change c) {
super(ea, c, "abandon");
public AbandonedSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, "abandon", newChangeData(ea, id));
}
@Override

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/AddReviewerSender.java
View File

@@ -16,18 +16,20 @@ package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
/** Asks a user to review a change. */
public class AddReviewerSender extends NewChangeSender {
public static interface Factory {
AddReviewerSender create(Change change);
AddReviewerSender create(Change.Id id);
}
@Inject
public AddReviewerSender(EmailArguments ea, @Assisted Change c) {
super(ea, c);
public AddReviewerSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, newChangeData(ea, id));
}
@Override

16
gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
View File

@@ -55,6 +55,10 @@ import java.util.TreeSet;
public abstract class ChangeEmail extends NotificationEmail {
private static final Logger log = LoggerFactory.getLogger(ChangeEmail.class);
protected static ChangeData newChangeData(EmailArguments ea, Change.Id id) {
return ea.changeDataFactory.create(ea.db.get(), id);
}
protected final Change change;
protected final ChangeData changeData;
protected PatchSet patchSet;
@@ -65,10 +69,11 @@ public abstract class ChangeEmail extends NotificationEmail {
protected Set<Account.Id> authors;
protected boolean emailOnlyAuthors;
protected ChangeEmail(EmailArguments ea, Change c, String mc) {
super(ea, mc, c.getProject(), c.getDest());
change = c;
changeData = ea.changeDataFactory.create(ea.db.get(), c);
protected ChangeEmail(EmailArguments ea, String mc, ChangeData cd)
throws OrmException {
super(ea, mc, cd.change().getDest());
changeData = cd;
change = cd.change();
emailOnlyAuthors = false;
}
@@ -305,7 +310,8 @@ public abstract class ChangeEmail extends NotificationEmail {
@Override
protected final Watchers getWatchers(NotifyType type) throws OrmException {
ProjectWatch watch = new ProjectWatch(args, project, projectState, changeData);
ProjectWatch watch = new ProjectWatch(
args, branch.getParentKey(), projectState, changeData);
return watch.getWatchers(type);
}

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java
View File

@@ -50,7 +50,7 @@ public class CommentSender extends ReplyToChangeSender {
.getLogger(CommentSender.class);
public static interface Factory {
public CommentSender create(NotifyHandling notify, Change change);
public CommentSender create(NotifyHandling notify, Change.Id id);
}
private final NotifyHandling notify;
@@ -59,10 +59,10 @@ public class CommentSender extends ReplyToChangeSender {
@Inject
public CommentSender(EmailArguments ea,
PatchLineCommentsUtil plcUtil,
@Assisted NotifyHandling notify,
@Assisted Change c,
PatchLineCommentsUtil plcUtil) {
super(ea, c, "comment");
@Assisted Change.Id id) throws OrmException {
super(ea, "comment", newChangeData(ea, id));
this.notify = notify;
this.plcUtil = plcUtil;
}

7
gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java
View File

@@ -33,12 +33,13 @@ public class CreateChangeSender extends NewChangeSender {
LoggerFactory.getLogger(CreateChangeSender.class);
public static interface Factory {
public CreateChangeSender create(Change change);
public CreateChangeSender create(Change.Id id);
}
@Inject
public CreateChangeSender(EmailArguments ea, @Assisted Change c) {
super(ea, c);
public CreateChangeSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, newChangeData(ea, id));
}
@Override

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/MergeFailSender.java
View File

@@ -16,18 +16,20 @@ package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
/** Send notice about a change failing to merged. */
public class MergeFailSender extends ReplyToChangeSender {
public static interface Factory {
public MergeFailSender create(Change change);
public MergeFailSender create(Change.Id id);
}
@Inject
public MergeFailSender(EmailArguments ea, @Assisted Change c) {
super(ea, c, "merge-failed");
public MergeFailSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, "merge-failed", newChangeData(ea, id));
}
@Override

11
gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java
View File

@@ -22,8 +22,8 @@ import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -31,15 +31,16 @@ import com.google.inject.assistedinject.Assisted;
/** Send notice about a change successfully merged. */
public class MergedSender extends ReplyToChangeSender {
public static interface Factory {
public MergedSender create(ChangeControl change);
public MergedSender create(Change.Id id);
}
private final LabelTypes labelTypes;
@Inject
public MergedSender(EmailArguments ea, @Assisted ChangeControl c) {
super(ea, c.getChange(), "merged");
labelTypes = c.getLabelTypes();
public MergedSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, "merged", newChangeData(ea, id));
labelTypes = changeData.changeControl().getLabelTypes();
}
@Override

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/NewChangeSender.java
View File

@@ -16,7 +16,8 @@ package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import java.util.ArrayList;
import java.util.Collection;
@@ -29,8 +30,9 @@ public abstract class NewChangeSender extends ChangeEmail {
private final Set<Account.Id> reviewers = new HashSet<>();
private final Set<Account.Id> extraCC = new HashSet<>();
protected NewChangeSender(EmailArguments ea, Change c) {
super(ea, c, "newchange");
protected NewChangeSender(EmailArguments ea, ChangeData cd)
throws OrmException {
super(ea, "newchange", cd);
}
public void addReviewers(final Collection<Account.Id> cc) {

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/NotificationEmail.java
View File

@@ -19,7 +19,6 @@ import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.mail.ProjectWatch.Watchers;
import com.google.gwtorm.server.OrmException;
@@ -33,14 +32,11 @@ public abstract class NotificationEmail extends OutgoingEmail {
private static final Logger log =
LoggerFactory.getLogger(NotificationEmail.class);
protected Project.NameKey project;
protected Branch.NameKey branch;
protected NotificationEmail(EmailArguments ea,
String mc, Project.NameKey project, Branch.NameKey branch) {
String mc, Branch.NameKey branch) {
super(ea, mc);
this.project = project;
this.branch = branch;
}
@@ -104,7 +100,7 @@ public abstract class NotificationEmail extends OutgoingEmail {
@Override
protected void setupVelocityContext() {
super.setupVelocityContext();
velocityContext.put("projectName", project.get());
velocityContext.put("projectName", branch.getParentKey().get());
velocityContext.put("branch", branch);
}
}

6
gerrit-server/src/main/java/com/google/gerrit/server/mail/PatchSetNotificationSender.java
View File

@@ -100,7 +100,8 @@ public class PatchSetNotificationSender {
updatedPatchSet, info, recipients.getReviewers(),
Collections.<Account.Id> emptySet());
try {
CreateChangeSender cm = createChangeSenderFactory.create(updatedChange);
CreateChangeSender cm =
createChangeSenderFactory.create(updatedChange.getId());
cm.setFrom(me);
cm.setPatchSet(updatedPatchSet, info);
cm.addReviewers(recipients.getReviewers());
@@ -119,7 +120,8 @@ public class PatchSetNotificationSender {
updatedPatchSet.getCreatedOn(), updatedPatchSet.getId());
msg.setMessage("Uploaded patch set " + updatedPatchSet.getPatchSetId() + ".");
try {
ReplacePatchSetSender cm = replacePatchSetFactory.create(updatedChange);
ReplacePatchSetSender cm =
replacePatchSetFactory.create(updatedChange.getId());
cm.setFrom(me);
cm.setPatchSet(updatedPatchSet, info);
cm.setChangeMessage(msg);

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplacePatchSetSender.java
View File

@@ -18,6 +18,7 @@ import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -30,15 +31,16 @@ import java.util.Set;
/** Send notice of new patch sets for reviewers. */
public class ReplacePatchSetSender extends ReplyToChangeSender {
public static interface Factory {
public ReplacePatchSetSender create(Change change);
public ReplacePatchSetSender create(Change.Id id);
}
private final Set<Account.Id> reviewers = new HashSet<>();
private final Set<Account.Id> extraCC = new HashSet<>();
@Inject
public ReplacePatchSetSender(EmailArguments ea, @Assisted Change c) {
super(ea, c, "newpatchset");
public ReplacePatchSetSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, "newpatchset", newChangeData(ea, id));
}
public void addReviewers(final Collection<Account.Id> cc) {

9
gerrit-server/src/main/java/com/google/gerrit/server/mail/ReplyToChangeSender.java
View File

@@ -16,15 +16,18 @@ package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
/** Alert a user to a reply to a change, usually commentary made during review. */
public abstract class ReplyToChangeSender extends ChangeEmail {
public static interface Factory<T extends ReplyToChangeSender> {
public T create(Change change);
public T create(Change.Id id);
}
protected ReplyToChangeSender(EmailArguments ea, Change c, String mc) {
super(ea, c, mc);
protected ReplyToChangeSender(EmailArguments ea, String mc, ChangeData cd)
throws OrmException {
super(ea, mc, cd);
}
@Override

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -25,12 +26,13 @@ public class RestoredSender extends ReplyToChangeSender {
public static interface Factory extends
ReplyToChangeSender.Factory<RestoredSender> {
@Override
RestoredSender create(Change change);
RestoredSender create(Change.Id id);
}
@Inject
public RestoredSender(EmailArguments ea, @Assisted Change c) {
super(ea, c, "restore");
public RestoredSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, "restore", newChangeData(ea, id));
}
@Override

8
gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java
View File

@@ -17,18 +17,20 @@ package com.google.gerrit.server.mail;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
/** Send notice about a change being reverted. */
public class RevertedSender extends ReplyToChangeSender {
public static interface Factory {
RevertedSender create(Change change);
RevertedSender create(Change.Id id);
}
@Inject
public RevertedSender(EmailArguments ea, @Assisted Change c) {
super(ea, c, "revert");
public RevertedSender(EmailArguments ea, @Assisted Change.Id id)
throws OrmException {
super(ea, "revert", newChangeData(ea, id));
}
@Override