Improve email sending

src/NuGetGallery.Core/Services/CoreMessageService.cs

@@ -2,11 +2,13 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.ObjectModel;
 using System.Collections.Generic;
 using System.Globalization;
 using System.Linq;
 using System.Net.Mail;
 using System.Text;
+using System.Threading.Tasks;
 using AnglicanGeek.MarkdownMailer;
 using NuGet.Services.Validation;
 using NuGet.Services.Validation.Issues;
@@ -15,9 +17,11 @@ namespace NuGetGallery.Services
 {
  public class CoreMessageService : ICoreMessageService
  {
- protected CoreMessageService()
- {
- }
+ private static readonly ReadOnlyCollection<TimeSpan> RetryDelays = Array.AsReadOnly(new[] {
+ TimeSpan.FromSeconds(0.1),
+ TimeSpan.FromSeconds(1),
+ TimeSpan.FromSeconds(10)
+ });
 
  public CoreMessageService(IMailSender mailSender, ICoreMessageServiceConfiguration coreConfiguration)
  {
@@ -28,7 +32,7 @@ public CoreMessageService(IMailSender mailSender, ICoreMessageServiceConfigurati
  public IMailSender MailSender { get; protected set; }
  public ICoreMessageServiceConfiguration CoreConfiguration { get; protected set; }
 
- public void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null)
+ public async Task SendPackageAddedNoticeAsync(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null)
  {
  bool hasWarnings = warningMessages != null && warningMessages.Any();
 
@@ -63,12 +67,12 @@ [change your email notification settings]({emailSettingsUrl}).
 
  if (mailMessage.To.Any())
  {
- SendMessage(mailMessage, copySender: false);
+ await SendMessageAsync(mailMessage);
  }
  }
  }
 
- public void SendPackageAddedWithWarningsNotice(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages)
+ public async Task SendPackageAddedWithWarningsNoticeAsync(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages)
  {
  var subject = $"[{CoreConfiguration.GalleryOwner.DisplayName}] Package pushed with warnings - {package.PackageRegistration.Id} {package.Version}";
  var warningMessagesPlaceholder = Environment.NewLine + string.Join(Environment.NewLine, warningMessages);
@@ -87,12 +91,12 @@ public void SendPackageAddedWithWarningsNotice(Package package, string packageUr
 
  if (mailMessage.To.Any())
  {
- SendMessage(mailMessage, copySender: false);
+ await SendMessageAsync(mailMessage);
  }
  }
  }
 
- public void SendPackageValidationFailedNotice(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl)
+ public async Task SendPackageValidationFailedNoticeAsync(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl)
  {
  var validationIssues = validationSet.GetValidationIssues();
 
@@ -133,7 +137,7 @@ public void SendPackageValidationFailedNotice(Package package, PackageValidation
 
  if (mailMessage.To.Any())
  {
- SendMessage(mailMessage, copySender: false);
+ await SendMessageAsync(mailMessage);
  }
  }
  }
@@ -169,7 +173,7 @@ private static string ParseValidationIssue(ValidationIssue validationIssue, stri
  }
  }
 
- public void SendValidationTakingTooLongNotice(Package package, string packageUrl)
+ public async Task SendValidationTakingTooLongNoticeAsync(Package package, string packageUrl)
  {
  string subject = "[{0}] Package validation taking longer than expected - {1} {2}";
  string body = "It is taking longer than expected for your package [{1} {2}]({3}) to get published.\n\n" +
@@ -201,7 +205,7 @@ public void SendValidationTakingTooLongNotice(Package package, string packageUrl
 
  if (mailMessage.To.Any())
  {
- SendMessage(mailMessage, copySender: false);
+ await SendMessageAsync(mailMessage);
  }
  }
  }
@@ -231,30 +235,54 @@ protected static void AddOwnersSubscribedToPackagePushedNotification(PackageRegi
  }
  }
 
- protected void SendMessage(MailMessage mailMessage)
+ protected virtual async Task SendMessageAsync(MailMessage mailMessage)
  {
- SendMessage(mailMessage, copySender: false);
+ int attempt = 0;
+ bool success = false;
+ while (!success)
+ {
+ try
+ {
+ await AttemptSendMessageAsync(mailMessage, attempt + 1);
+ success = true;
+ }
+ catch (SmtpException)
+ {
+ if (attempt < RetryDelays.Count)
+ {
+ await Task.Delay(RetryDelays[attempt]);
+ attempt++;
+ }
+ else
+ {
+ throw;
+ }
+ }
+ }
  }
 
- virtual protected void SendMessage(MailMessage mailMessage, bool copySender)
+ protected virtual Task AttemptSendMessageAsync(MailMessage mailMessage, int attemptNumber)
  {
+ // AnglicanGeek.MarkdownMailer doesn't have an async overload
  MailSender.Send(mailMessage);
- if (copySender)
+ return Task.CompletedTask;
+ }
+
+ protected async Task SendMessageToSenderAsync(MailMessage mailMessage)
+ {
+ using (var senderCopy = new MailMessage(
+ CoreConfiguration.GalleryOwner,
+ mailMessage.ReplyToList.First()))
  {
- var senderCopy = new MailMessage(
- CoreConfiguration.GalleryOwner,
- mailMessage.ReplyToList.First())
- {
- Subject = mailMessage.Subject + " [Sender Copy]",
- Body = string.Format(
- CultureInfo.CurrentCulture,
- "You sent the following message via {0}: {1}{1}{2}",
- CoreConfiguration.GalleryOwner.DisplayName,
- Environment.NewLine,
- mailMessage.Body),
- };
+ senderCopy.Subject = mailMessage.Subject + " [Sender Copy]";
+ senderCopy.Body = string.Format(
+ CultureInfo.CurrentCulture,
+ "You sent the following message via {0}: {1}{1}{2}",
+ CoreConfiguration.GalleryOwner.DisplayName,
+ Environment.NewLine,
+ mailMessage.Body);
  senderCopy.ReplyToList.Add(mailMessage.ReplyToList.First());
- MailSender.Send(senderCopy);
+ await SendMessageAsync(senderCopy);
  }
  }
  }

src/NuGetGallery.Core/Services/ICoreMessageService.cs

@@ -3,14 +3,15 @@
 
 using System.Collections.Generic;
 using NuGet.Services.Validation;
+using System.Threading.Tasks;
 
 namespace NuGetGallery.Services
 {
  public interface ICoreMessageService
  {
- void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null);
- void SendPackageAddedWithWarningsNotice(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages);
- void SendPackageValidationFailedNotice(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl);
- void SendValidationTakingTooLongNotice(Package package, string packageUrl);
+ Task SendPackageAddedNoticeAsync(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null);
+ Task SendPackageAddedWithWarningsNoticeAsync(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages);
+ Task SendPackageValidationFailedNoticeAsync(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl);
+ Task SendValidationTakingTooLongNoticeAsync(Package package, string packageUrl);
  }
 }

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

@@ -35,6 +35,7 @@
 using NuGetGallery.Infrastructure.Authentication;
 using NuGetGallery.Infrastructure.Lucene;
 using NuGetGallery.Security;
+using NuGetGallery.Services;
 using SecretReaderFactory = NuGetGallery.Configuration.SecretReader.SecretReaderFactory;
 
 namespace NuGetGallery
@@ -356,7 +357,7 @@ protected override void Load(ContainerBuilder builder)
  .As<IMailSender>()
  .InstancePerLifetimeScope();
 
- builder.RegisterType<MessageService>()
+ builder.RegisterType<BackgroundMessageService>()
  .AsSelf()
  .As<IMessageService>()
  .InstancePerLifetimeScope();

src/NuGetGallery/Controllers/AccountsController.cs

@@ -92,7 +92,7 @@ public virtual ActionResult ConfirmationRequired(string accountName = null)
  [HttpPost]
  [ActionName("ConfirmationRequired")]
  [ValidateAntiForgeryToken]
- public virtual ActionResult ConfirmationRequiredPost(string accountName = null)
+ public virtual async Task<ActionResult> ConfirmationRequiredPost(string accountName = null)
  {
  var account = GetAccount(accountName);
 
@@ -108,7 +108,7 @@ public virtual ActionResult ConfirmationRequiredPost(string accountName = null)
  ConfirmationViewModel model;
  if (!alreadyConfirmed)
  {
- SendNewAccountEmail(account);
+ await SendNewAccountEmailAsync(account);
 
  model = new ConfirmationViewModel(account)
  {
@@ -122,7 +122,7 @@ public virtual ActionResult ConfirmationRequiredPost(string accountName = null)
  return View(model);
  }
 
- protected abstract void SendNewAccountEmail(User account);
+ protected abstract Task SendNewAccountEmailAsync(User account);
 
  [UIAuthorize(allowDiscontinuedLogins: true)]
  public virtual async Task<ActionResult> Confirm(string accountName, string token)
@@ -163,7 +163,7 @@ public virtual async Task<ActionResult> Confirm(string accountName, string token
  // Change notice not required for new accounts.
  if (model.SuccessfulConfirmation && !model.ConfirmingNewAccount)
  {
- MessageService.SendEmailChangeNoticeToPreviousEmailAddress(account, existingEmail);
+ await MessageService.SendEmailChangeNoticeToPreviousEmailAddressAsync(account, existingEmail);
 
  string returnUrl = HttpContext.GetConfirmationReturnUrl();
  if (!String.IsNullOrEmpty(returnUrl))
@@ -254,13 +254,13 @@ public virtual async Task<ActionResult> ChangeEmail(TAccountViewModel model)
 
  if (account.Confirmed)
  {
- SendEmailChangedConfirmationNotice(account);
+ await SendEmailChangedConfirmationNoticeAsync(account);
  }
 
  return RedirectToAction(AccountAction);
  }
 
- protected abstract void SendEmailChangedConfirmationNotice(User account);
+ protected abstract Task SendEmailChangedConfirmationNoticeAsync(User account);
 
  [HttpPost]
  [UIAuthorize]

src/NuGetGallery/Controllers/ApiController.cs

@@ -710,7 +710,7 @@ await AuditingService.SaveAuditRecordAsync(
  if (!(ConfigurationService.Current.AsynchronousPackageValidationEnabled && ConfigurationService.Current.BlockingAsynchronousPackageValidationEnabled))
  {
  // Notify user of push unless async validation in blocking mode is used
- MessageService.SendPackageAddedNotice(package,
+ await MessageService.SendPackageAddedNoticeAsync(package,
  Url.Package(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
  Url.ReportPackage(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
  Url.AccountSettings(relativeUrl: false),
@@ -720,7 +720,7 @@ await AuditingService.SaveAuditRecordAsync(
  else if (packagePolicyResult.HasWarnings)
  {
  // Notify user of push unless async validation in blocking mode is used
- MessageService.SendPackageAddedWithWarningsNotice(package,
+ await MessageService.SendPackageAddedWithWarningsNoticeAsync(package,
  Url.Package(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
  Url.ReportPackage(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
  packagePolicyResult.WarningMessages);

src/NuGetGallery/Controllers/AuthenticationController.cs

@@ -287,7 +287,7 @@ public virtual async Task<ActionResult> Register(LogOnViewModel model, string re
  // Send a new account email
  if (NuGetContext.Config.Current.ConfirmEmailAddresses && !string.IsNullOrEmpty(user.User.UnconfirmedEmailAddress))
  {
- _messageService.SendNewAccountEmail(
+ await _messageService.SendNewAccountEmailAsync(
  user.User,
  Url.ConfirmEmail(
  user.User.Username,
@@ -325,7 +325,7 @@ public virtual ActionResult LogOff(string returnUrl)
 
  [HttpPost]
  [ValidateAntiForgeryToken]
- public virtual JsonResult SignInAssistance(string username, string providedEmailAddress)
+ public virtual async Task<JsonResult> SignInAssistance(string username, string providedEmailAddress)
  {
  // If provided email address is empty or null, return the result with a formatted
  // email address, otherwise send sign-in assistance email to the associated mail address.
@@ -352,7 +352,7 @@ public virtual JsonResult SignInAssistance(string username, string providedEmail
  else
  {
  var externalCredentials = user.Credentials.Where(cred => cred.IsExternal());
- _messageService.SendSigninAssistanceEmail(new MailAddress(email, user.Username), externalCredentials);
+ await _messageService.SendSigninAssistanceEmailAsync(new MailAddress(email, user.Username), externalCredentials);
  return Json(new { success = true });
  }
  }
@@ -674,7 +674,7 @@ private async Task<LoginUserDetails> AssociateCredential(AuthenticatedUser user)
  await RemovePasswordCredential(user.User);
 
  // Notify the user of the change
- _messageService.SendCredentialAddedNotice(user.User, _authService.DescribeCredential(result.Credential));
+ await _messageService.SendCredentialAddedNoticeAsync(user.User, _authService.DescribeCredential(result.Credential));
 
  return new LoginUserDetails
  {

src/NuGetGallery/Controllers/JsonApiController.cs

@@ -119,7 +119,7 @@ public async Task<JsonResult> AddPackageOwner(string id, string username, string
 
  foreach (var owner in model.Package.Owners)
  {
- _messageService.SendPackageOwnerAddedNotice(owner, model.User, model.Package, packageUrl);
+ await _messageService.SendPackageOwnerAddedNoticeAsync(owner, model.User, model.Package, packageUrl);
  }
  }
  else
@@ -147,12 +147,12 @@ public async Task<JsonResult> AddPackageOwner(string id, string username, string
  model.User.Username,
  relativeUrl: false);
 
- _messageService.SendPackageOwnerRequest(model.CurrentUser, model.User, model.Package, packageUrl,
+ await _messageService.SendPackageOwnerRequestAsync(model.CurrentUser, model.User, model.Package, packageUrl,
  confirmationUrl, rejectionUrl, encodedMessage, policyMessage: string.Empty);
 
  foreach (var owner in model.Package.Owners)
  {
- _messageService.SendPackageOwnerRequestInitiatedNotice(model.CurrentUser, owner, model.User, model.Package, cancellationUrl);
+ await _messageService.SendPackageOwnerRequestInitiatedNoticeAsync(model.CurrentUser, owner, model.User, model.Package, cancellationUrl);
  }
  }
 
@@ -190,12 +190,12 @@ public async Task<JsonResult> RemovePackageOwner(string id, string username)
  throw new InvalidOperationException("You can't remove the only owner from a package.");
  }
  await _packageOwnershipManagementService.RemovePackageOwnerAsync(model.Package, model.CurrentUser, model.User, commitAsTransaction:true);
- _messageService.SendPackageOwnerRemovedNotice(model.CurrentUser, model.User, model.Package);
+ await _messageService.SendPackageOwnerRemovedNoticeAsync(model.CurrentUser, model.User, model.Package);
  }
  else
  {
  await _packageOwnershipManagementService.DeletePackageOwnershipRequestAsync(model.Package, model.User);
- _messageService.SendPackageOwnerRequestCancellationNotice(model.CurrentUser, model.User, model.Package);
+ await _messageService.SendPackageOwnerRequestCancellationNoticeAsync(model.CurrentUser, model.User, model.Package);
  }
 
  return Json(new { success = true });