Merge changes I8ed34f11,I4aed3276,I4bcdcb50
* changes:
AttentionSetEmail: Use fromChangeUpdateAndReason to generate messageId
Move messageId generation into AttentionSetEmail
AttentionSetEmail: Avoid accessing non-thread-safe vars from bg thread
diff --git a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
index a980c32..1cf31c1 100644
--- a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
@@ -16,13 +16,11 @@
import static java.util.Objects.requireNonNull;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.mail.send.AddToAttentionSetSender;
-import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -31,18 +29,14 @@
import com.google.gerrit.server.util.AttentionSetEmail;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
-import java.io.IOException;
/** Add a specified user to the attention set. */
public class AddToAttentionSetOp implements BatchUpdateOp {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
public interface Factory {
AddToAttentionSetOp create(Account.Id attentionUserId, String reason, boolean notify);
}
private final ChangeData.Factory changeDataFactory;
- private final MessageIdGenerator messageIdGenerator;
private final AddToAttentionSetSender.Factory addToAttentionSetSender;
private final AttentionSetEmail.Factory attentionSetEmailFactory;
@@ -63,14 +57,12 @@
AddToAttentionSetOp(
ChangeData.Factory changeDataFactory,
AddToAttentionSetSender.Factory addToAttentionSetSender,
- MessageIdGenerator messageIdGenerator,
AttentionSetEmail.Factory attentionSetEmailFactory,
@Assisted Account.Id attentionUserId,
@Assisted String reason,
@Assisted boolean notify) {
this.changeDataFactory = changeDataFactory;
this.addToAttentionSetSender = addToAttentionSetSender;
- this.messageIdGenerator = messageIdGenerator;
this.attentionSetEmailFactory = attentionSetEmailFactory;
this.attentionUserId = requireNonNull(attentionUserId, "user");
@@ -105,18 +97,13 @@
if (!notify) {
return;
}
- try {
- attentionSetEmailFactory
- .create(
- addToAttentionSetSender.create(ctx.getProject(), change.getId()),
- ctx,
- change,
- reason,
- messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()),
- attentionUserId)
- .sendAsync();
- } catch (IOException e) {
- logger.atSevere().withCause(e).log(e.getMessage(), change.getId());
- }
+ attentionSetEmailFactory
+ .create(
+ addToAttentionSetSender.create(ctx.getProject(), change.getId()),
+ ctx,
+ change,
+ reason,
+ attentionUserId)
+ .sendAsync();
}
}
diff --git a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
index 50ee9d4..9fb4fc4 100644
--- a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
@@ -16,13 +16,11 @@
import static java.util.Objects.requireNonNull;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.AttentionSetUpdate.Operation;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.mail.send.RemoveFromAttentionSetSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.query.change.ChangeData;
@@ -32,19 +30,15 @@
import com.google.gerrit.server.util.AttentionSetEmail;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
-import java.io.IOException;
import java.util.Optional;
/** Remove a specified user from the attention set. */
public class RemoveFromAttentionSetOp implements BatchUpdateOp {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
public interface Factory {
RemoveFromAttentionSetOp create(Account.Id attentionUserId, String reason, boolean notify);
}
private final ChangeData.Factory changeDataFactory;
- private final MessageIdGenerator messageIdGenerator;
private final RemoveFromAttentionSetSender.Factory removeFromAttentionSetSender;
private final AttentionSetEmail.Factory attentionSetEmailFactory;
@@ -64,14 +58,12 @@
@Inject
RemoveFromAttentionSetOp(
ChangeData.Factory changeDataFactory,
- MessageIdGenerator messageIdGenerator,
RemoveFromAttentionSetSender.Factory removeFromAttentionSetSenderFactory,
AttentionSetEmail.Factory attentionSetEmailFactory,
@Assisted Account.Id attentionUserId,
@Assisted String reason,
@Assisted boolean notify) {
this.changeDataFactory = changeDataFactory;
- this.messageIdGenerator = messageIdGenerator;
this.removeFromAttentionSetSender = removeFromAttentionSetSenderFactory;
this.attentionSetEmailFactory = attentionSetEmailFactory;
this.attentionUserId = requireNonNull(attentionUserId, "user");
@@ -105,18 +97,13 @@
if (!notify) {
return;
}
- try {
- attentionSetEmailFactory
- .create(
- removeFromAttentionSetSender.create(ctx.getProject(), change.getId()),
- ctx,
- change,
- reason,
- messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()),
- attentionUserId)
- .sendAsync();
- } catch (IOException e) {
- logger.atSevere().withCause(e).log(e.getMessage(), change.getId());
- }
+ attentionSetEmailFactory
+ .create(
+ removeFromAttentionSetSender.create(ctx.getProject(), change.getId()),
+ ctx,
+ change,
+ reason,
+ attentionUserId)
+ .sendAsync();
}
}
diff --git a/java/com/google/gerrit/server/util/AttentionSetEmail.java b/java/com/google/gerrit/server/util/AttentionSetEmail.java
index 22ab62a..1b36139 100644
--- a/java/com/google/gerrit/server/util/AttentionSetEmail.java
+++ b/java/com/google/gerrit/server/util/AttentionSetEmail.java
@@ -18,19 +18,24 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.AddToAttentionSetSender;
import com.google.gerrit.server.mail.send.AttentionSetSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
+import com.google.gerrit.server.mail.send.MessageIdGenerator.MessageId;
import com.google.gerrit.server.mail.send.RemoveFromAttentionSetSender;
import com.google.gerrit.server.update.Context;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.io.IOException;
+import java.io.UncheckedIOException;
import java.util.Optional;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
-public class AttentionSetEmail implements Runnable, RequestContext {
+public class AttentionSetEmail {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -43,7 +48,6 @@
* @param ctx context for sending the email.
* @param change the change that the user was added/removed in.
* @param reason reason for adding/removing the user.
- * @param messageId messageId for tracking the email.
* @param attentionUserId the user added/removed.
*/
AttentionSetEmail create(
@@ -51,77 +55,117 @@
Context ctx,
Change change,
String reason,
- MessageIdGenerator.MessageId messageId,
Account.Id attentionUserId);
}
private final ExecutorService sendEmailsExecutor;
- private final ThreadLocalRequestContext requestContext;
- private final AccountTemplateUtil accountTemplateUtil;
- private final AttentionSetSender sender;
- private final Context ctx;
- private final Change change;
- private final String reason;
- private final MessageIdGenerator.MessageId messageId;
- private final Account.Id attentionUserId;
+ private final AsyncSender asyncSender;
@Inject
AttentionSetEmail(
@SendEmailExecutor ExecutorService executor,
ThreadLocalRequestContext requestContext,
+ MessageIdGenerator messageIdGenerator,
AccountTemplateUtil accountTemplateUtil,
@Assisted AttentionSetSender sender,
@Assisted Context ctx,
@Assisted Change change,
@Assisted String reason,
- @Assisted MessageIdGenerator.MessageId messageId,
@Assisted Account.Id attentionUserId) {
this.sendEmailsExecutor = executor;
- this.requestContext = requestContext;
- this.accountTemplateUtil = accountTemplateUtil;
- this.sender = sender;
- this.ctx = ctx;
- this.change = change;
- this.reason = reason;
- this.messageId = messageId;
- this.attentionUserId = attentionUserId;
+
+ MessageId messageId;
+ try {
+ messageId =
+ messageIdGenerator.fromChangeUpdateAndReason(
+ ctx.getRepoView(), change.currentPatchSetId(), "AttentionSetEmail");
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ this.asyncSender =
+ new AsyncSender(
+ requestContext,
+ ctx.getIdentifiedUser(),
+ sender,
+ messageId,
+ ctx.getNotify(change.getId()),
+ attentionUserId,
+ accountTemplateUtil.replaceTemplates(reason),
+ change.getId());
}
public void sendAsync() {
@SuppressWarnings("unused")
- Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
+ Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(asyncSender);
}
- @Override
- public void run() {
- RequestContext old = requestContext.setContext(this);
- try {
- Optional<Account.Id> accountId =
- ctx.getUser().isIdentifiedUser()
- ? Optional.of(ctx.getUser().asIdentifiedUser().getAccountId())
- : Optional.empty();
- if (accountId.isPresent()) {
- sender.setFrom(accountId.get());
- }
- sender.setNotify(ctx.getNotify(change.getId()));
- sender.setAttentionSetUser(attentionUserId);
- sender.setReason(accountTemplateUtil.replaceTemplates(reason));
- sender.setMessageId(messageId);
- sender.send();
- } catch (Exception e) {
- logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
- } finally {
- requestContext.setContext(old);
+ /**
+ * {@link Runnable} that sends the email asynchonously.
+ *
+ * <p>Only pass objects into this class that are thread-safe (e.g. immutable) so that they can be
+ * safely accessed from the background thread.
+ */
+ private static class AsyncSender implements Runnable, RequestContext {
+ private final ThreadLocalRequestContext requestContext;
+ private final IdentifiedUser user;
+ private final AttentionSetSender sender;
+ private final MessageIdGenerator.MessageId messageId;
+ private final NotifyResolver.Result notify;
+ private final Account.Id attentionUserId;
+ private final String reason;
+ private final Change.Id changeId;
+
+ AsyncSender(
+ ThreadLocalRequestContext requestContext,
+ IdentifiedUser user,
+ AttentionSetSender sender,
+ MessageIdGenerator.MessageId messageId,
+ NotifyResolver.Result notify,
+ Account.Id attentionUserId,
+ String reason,
+ Change.Id changeId) {
+ this.requestContext = requestContext;
+ this.user = user;
+ this.sender = sender;
+ this.messageId = messageId;
+ this.notify = notify;
+ this.attentionUserId = attentionUserId;
+ this.reason = reason;
+ this.changeId = changeId;
}
- }
- @Override
- public String toString() {
- return "send-email attention-set-update";
- }
+ @Override
+ public void run() {
+ RequestContext old = requestContext.setContext(this);
+ try {
+ Optional<Account.Id> accountId =
+ user.isIdentifiedUser()
+ ? Optional.of(user.asIdentifiedUser().getAccountId())
+ : Optional.empty();
+ if (accountId.isPresent()) {
+ sender.setFrom(accountId.get());
+ }
+ sender.setNotify(notify);
+ sender.setAttentionSetUser(attentionUserId);
+ sender.setReason(reason);
+ sender.setMessageId(messageId);
+ sender.send();
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log("Cannot email update for change %s", changeId);
+ } finally {
+ requestContext.setContext(old);
+ }
+ }
- @Override
- public CurrentUser getUser() {
- return ctx.getUser();
+ @Override
+ public String toString() {
+ return "send-email attention-set-update";
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return user;
+ }
}
}
