Skip to content

Commit baa5bc0

Browse files
zivkanzivkan
authored
Retry failed emails (#6312)
Retry sending emails on SmtpExceptions. Record telemetry on each attempt to send an email. Send emails in the background, to avoid making the user wait for retries.
1 parent 665daa6 commit baa5bc0

29 files changed

Lines changed: 720 additions & 447 deletions

‎src/NuGetGallery.Core/Services/CoreMessageService.cs‎

Lines changed: 56 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using System.Collections.ObjectModel;
56
using System.Collections.Generic;
67
using System.Globalization;
78
using System.Linq;
89
using System.Net.Mail;
910
using System.Text;
11+
using System.Threading.Tasks;
1012
using AnglicanGeek.MarkdownMailer;
1113
using NuGet.Services.Validation;
1214
using NuGet.Services.Validation.Issues;
@@ -15,9 +17,11 @@ namespace NuGetGallery.Services
1517
{
1618
public class CoreMessageService : ICoreMessageService
1719
{
18-
protected CoreMessageService()
19-
{
20-
}
20+
private static readonly ReadOnlyCollection<TimeSpan> RetryDelays = Array.AsReadOnly(new[] {
21+
TimeSpan.FromSeconds(0.1),
22+
TimeSpan.FromSeconds(1),
23+
TimeSpan.FromSeconds(10)
24+
});
2125

2226
public CoreMessageService(IMailSender mailSender, ICoreMessageServiceConfiguration coreConfiguration)
2327
{
@@ -28,7 +32,7 @@ public CoreMessageService(IMailSender mailSender, ICoreMessageServiceConfigurati
2832
public IMailSender MailSender { get; protected set; }
2933
public ICoreMessageServiceConfiguration CoreConfiguration { get; protected set; }
3034

31-
public void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null)
35+
public async Task SendPackageAddedNoticeAsync(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null)
3236
{
3337
bool hasWarnings = warningMessages != null && warningMessages.Any();
3438

@@ -63,12 +67,12 @@ [change your email notification settings]({emailSettingsUrl}).
6367

6468
if (mailMessage.To.Any())
6569
{
66-
SendMessage(mailMessage);
70+
await SendMessageAsync(mailMessage);
6771
}
6872
}
6973
}
7074

71-
public void SendPackageAddedWithWarningsNotice(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages)
75+
public async Task SendPackageAddedWithWarningsNoticeAsync(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages)
7276
{
7377
var subject = $"[{CoreConfiguration.GalleryOwner.DisplayName}] Package pushed with warnings - {package.PackageRegistration.Id} {package.Version}";
7478
var warningMessagesPlaceholder = Environment.NewLine + string.Join(Environment.NewLine, warningMessages);
@@ -87,12 +91,12 @@ public void SendPackageAddedWithWarningsNotice(Package package, string packageUr
8791

8892
if (mailMessage.To.Any())
8993
{
90-
SendMessage(mailMessage);
94+
await SendMessageAsync(mailMessage);
9195
}
9296
}
9397
}
9498

95-
public void SendPackageValidationFailedNotice(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl)
99+
public async Task SendPackageValidationFailedNoticeAsync(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl)
96100
{
97101
var validationIssues = validationSet.GetValidationIssues();
98102

@@ -133,7 +137,7 @@ public void SendPackageValidationFailedNotice(Package package, PackageValidation
133137

134138
if (mailMessage.To.Any())
135139
{
136-
SendMessage(mailMessage, copySender: false);
140+
await SendMessageAsync(mailMessage);
137141
}
138142
}
139143
}
@@ -169,7 +173,7 @@ private static string ParseValidationIssue(ValidationIssue validationIssue, stri
169173
}
170174
}
171175

172-
public void SendValidationTakingTooLongNotice(Package package, string packageUrl)
176+
public async Task SendValidationTakingTooLongNoticeAsync(Package package, string packageUrl)
173177
{
174178
string subject = "[{0}] Package validation taking longer than expected - {1} {2}";
175179
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
201205

202206
if (mailMessage.To.Any())
203207
{
204-
SendMessage(mailMessage, copySender: false);
208+
await SendMessageAsync(mailMessage);
205209
}
206210
}
207211
}
@@ -231,30 +235,54 @@ protected static void AddOwnersSubscribedToPackagePushedNotification(PackageRegi
231235
}
232236
}
233237

234-
protected void SendMessage(MailMessage mailMessage)
238+
protected virtual async Task SendMessageAsync(MailMessage mailMessage)
235239
{
236-
SendMessage(mailMessage, copySender: false);
240+
int attempt = 0;
241+
bool success = false;
242+
while (!success)
243+
{
244+
try
245+
{
246+
await AttemptSendMessageAsync(mailMessage, attempt + 1);
247+
success = true;
248+
}
249+
catch (SmtpException)
250+
{
251+
if (attempt < RetryDelays.Count)
252+
{
253+
await Task.Delay(RetryDelays[attempt]);
254+
attempt++;
255+
}
256+
else
257+
{
258+
throw;
259+
}
260+
}
261+
}
237262
}
238263

239-
virtual protected void SendMessage(MailMessage mailMessage, bool copySender)
264+
protected virtual Task AttemptSendMessageAsync(MailMessage mailMessage, int attemptNumber)
240265
{
266+
// AnglicanGeek.MarkdownMailer doesn't have an async overload
241267
MailSender.Send(mailMessage);
242-
if (copySender)
268+
return Task.CompletedTask;
269+
}
270+
271+
protected async Task SendMessageToSenderAsync(MailMessage mailMessage)
272+
{
273+
using (var senderCopy = new MailMessage(
274+
CoreConfiguration.GalleryOwner,
275+
mailMessage.ReplyToList.First()))
243276
{
244-
var senderCopy = new MailMessage(
245-
CoreConfiguration.GalleryOwner,
246-
mailMessage.ReplyToList.First())
247-
{
248-
Subject = mailMessage.Subject + " [Sender Copy]",
249-
Body = string.Format(
250-
CultureInfo.CurrentCulture,
251-
"You sent the following message via {0}: {1}{1}{2}",
252-
CoreConfiguration.GalleryOwner.DisplayName,
253-
Environment.NewLine,
254-
mailMessage.Body),
255-
};
277+
senderCopy.Subject = mailMessage.Subject + " [Sender Copy]";
278+
senderCopy.Body = string.Format(
279+
CultureInfo.CurrentCulture,
280+
"You sent the following message via {0}: {1}{1}{2}",
281+
CoreConfiguration.GalleryOwner.DisplayName,
282+
Environment.NewLine,
283+
mailMessage.Body);
256284
senderCopy.ReplyToList.Add(mailMessage.ReplyToList.First());
257-
MailSender.Send(senderCopy);
285+
await SendMessageAsync(senderCopy);
258286
}
259287
}
260288
}

‎src/NuGetGallery.Core/Services/ICoreMessageService.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,15 @@
33

44
using System.Collections.Generic;
55
using NuGet.Services.Validation;
6+
using System.Threading.Tasks;
67

78
namespace NuGetGallery.Services
89
{
910
public interface ICoreMessageService
1011
{
11-
void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null);
12-
void SendPackageAddedWithWarningsNotice(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages);
13-
void SendPackageValidationFailedNotice(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl);
14-
void SendValidationTakingTooLongNotice(Package package, string packageUrl);
12+
Task SendPackageAddedNoticeAsync(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null);
13+
Task SendPackageAddedWithWarningsNoticeAsync(Package package, string packageUrl, string packageSupportUrl, IEnumerable<string> warningMessages);
14+
Task SendPackageValidationFailedNoticeAsync(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl);
15+
Task SendValidationTakingTooLongNoticeAsync(Package package, string packageUrl);
1516
}
1617
}

‎src/NuGetGallery/App_Start/DefaultDependenciesModule.cs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
using NuGetGallery.Infrastructure.Authentication;
3636
using NuGetGallery.Infrastructure.Lucene;
3737
using NuGetGallery.Security;
38+
using NuGetGallery.Services;
3839
using SecretReaderFactory = NuGetGallery.Configuration.SecretReader.SecretReaderFactory;
3940

4041
namespace NuGetGallery
@@ -362,12 +363,12 @@ protected override void Load(ContainerBuilder builder)
362363
builder.Register(c => mailSenderFactory())
363364
.AsSelf()
364365
.As<IMailSender>()
365-
.InstancePerLifetimeScope();
366+
.InstancePerDependency();
366367

367-
builder.RegisterType<MessageService>()
368+
builder.RegisterType<BackgroundMessageService>()
368369
.AsSelf()
369370
.As<IMessageService>()
370-
.InstancePerLifetimeScope();
371+
.InstancePerDependency();
371372

372373
builder.Register(c => HttpContext.Current.User)
373374
.AsSelf()

‎src/NuGetGallery/Controllers/AccountsController.cs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ public virtual ActionResult ConfirmationRequired(string accountName = null)
9292
[HttpPost]
9393
[ActionName("ConfirmationRequired")]
9494
[ValidateAntiForgeryToken]
95-
public virtual ActionResult ConfirmationRequiredPost(string accountName = null)
95+
public virtual async Task<ActionResult> ConfirmationRequiredPost(string accountName = null)
9696
{
9797
var account = GetAccount(accountName);
9898

@@ -108,7 +108,7 @@ public virtual ActionResult ConfirmationRequiredPost(string accountName = null)
108108
ConfirmationViewModel model;
109109
if (!alreadyConfirmed)
110110
{
111-
SendNewAccountEmail(account);
111+
await SendNewAccountEmailAsync(account);
112112

113113
model = new ConfirmationViewModel(account)
114114
{
@@ -122,7 +122,7 @@ public virtual ActionResult ConfirmationRequiredPost(string accountName = null)
122122
return View(model);
123123
}
124124

125-
protected abstract void SendNewAccountEmail(User account);
125+
protected abstract Task SendNewAccountEmailAsync(User account);
126126

127127
[UIAuthorize(allowDiscontinuedLogins: true)]
128128
public virtual async Task<ActionResult> Confirm(string accountName, string token)
@@ -163,7 +163,7 @@ public virtual async Task<ActionResult> Confirm(string accountName, string token
163163
// Change notice not required for new accounts.
164164
if (model.SuccessfulConfirmation && !model.ConfirmingNewAccount)
165165
{
166-
MessageService.SendEmailChangeNoticeToPreviousEmailAddress(account, existingEmail);
166+
await MessageService.SendEmailChangeNoticeToPreviousEmailAddressAsync(account, existingEmail);
167167

168168
string returnUrl = HttpContext.GetConfirmationReturnUrl();
169169
if (!String.IsNullOrEmpty(returnUrl))
@@ -254,13 +254,13 @@ public virtual async Task<ActionResult> ChangeEmail(TAccountViewModel model)
254254

255255
if (account.Confirmed && !string.IsNullOrEmpty(account.UnconfirmedEmailAddress))
256256
{
257-
SendEmailChangedConfirmationNotice(account);
257+
await SendEmailChangedConfirmationNoticeAsync(account);
258258
}
259259

260260
return RedirectToAction(AccountAction);
261261
}
262262

263-
protected abstract void SendEmailChangedConfirmationNotice(User account);
263+
protected abstract Task SendEmailChangedConfirmationNoticeAsync(User account);
264264

265265
[HttpPost]
266266
[UIAuthorize]

‎src/NuGetGallery/Controllers/ApiController.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,7 @@ await AuditingService.SaveAuditRecordAsync(
713713
if (!(ConfigurationService.Current.AsynchronousPackageValidationEnabled && ConfigurationService.Current.BlockingAsynchronousPackageValidationEnabled))
714714
{
715715
// Notify user of push unless async validation in blocking mode is used
716-
MessageService.SendPackageAddedNotice(package,
716+
await MessageService.SendPackageAddedNoticeAsync(package,
717717
Url.Package(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
718718
Url.ReportPackage(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
719719
Url.AccountSettings(relativeUrl: false),
@@ -723,7 +723,7 @@ await AuditingService.SaveAuditRecordAsync(
723723
else if (packagePolicyResult.HasWarnings)
724724
{
725725
// Notify user of push unless async validation in blocking mode is used
726-
MessageService.SendPackageAddedWithWarningsNotice(package,
726+
await MessageService.SendPackageAddedWithWarningsNoticeAsync(package,
727727
Url.Package(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
728728
Url.ReportPackage(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
729729
packagePolicyResult.WarningMessages);

‎src/NuGetGallery/Controllers/AuthenticationController.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ public virtual async Task<ActionResult> Register(LogOnViewModel model, string re
287287
// Send a new account email
288288
if (NuGetContext.Config.Current.ConfirmEmailAddresses && !string.IsNullOrEmpty(user.User.UnconfirmedEmailAddress))
289289
{
290-
_messageService.SendNewAccountEmail(
290+
await _messageService.SendNewAccountEmailAsync(
291291
user.User,
292292
Url.ConfirmEmail(
293293
user.User.Username,
@@ -325,7 +325,7 @@ public virtual ActionResult LogOff(string returnUrl)
325325

326326
[HttpPost]
327327
[ValidateAntiForgeryToken]
328-
public virtual JsonResult SignInAssistance(string username, string providedEmailAddress)
328+
public virtual async Task<JsonResult> SignInAssistance(string username, string providedEmailAddress)
329329
{
330330
// If provided email address is empty or null, return the result with a formatted
331331
// 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
352352
else
353353
{
354354
var externalCredentials = user.Credentials.Where(cred => cred.IsExternal());
355-
_messageService.SendSigninAssistanceEmail(new MailAddress(email, user.Username), externalCredentials);
355+
await _messageService.SendSigninAssistanceEmailAsync(new MailAddress(email, user.Username), externalCredentials);
356356
return Json(new { success = true });
357357
}
358358
}
@@ -674,7 +674,7 @@ private async Task<LoginUserDetails> AssociateCredential(AuthenticatedUser user)
674674
await RemovePasswordCredential(user.User);
675675

676676
// Notify the user of the change
677-
_messageService.SendCredentialAddedNotice(user.User, _authService.DescribeCredential(result.Credential));
677+
await _messageService.SendCredentialAddedNoticeAsync(user.User, _authService.DescribeCredential(result.Credential));
678678

679679
return new LoginUserDetails
680680
{

‎src/NuGetGallery/Controllers/JsonApiController.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ public async Task<JsonResult> AddPackageOwner(string id, string username, string
119119

120120
foreach (var owner in model.Package.Owners)
121121
{
122-
_messageService.SendPackageOwnerAddedNotice(owner, model.User, model.Package, packageUrl);
122+
await _messageService.SendPackageOwnerAddedNoticeAsync(owner, model.User, model.Package, packageUrl);
123123
}
124124
}
125125
else
@@ -147,12 +147,12 @@ public async Task<JsonResult> AddPackageOwner(string id, string username, string
147147
model.User.Username,
148148
relativeUrl: false);
149149

150-
_messageService.SendPackageOwnerRequest(model.CurrentUser, model.User, model.Package, packageUrl,
150+
await _messageService.SendPackageOwnerRequestAsync(model.CurrentUser, model.User, model.Package, packageUrl,
151151
confirmationUrl, rejectionUrl, encodedMessage, policyMessage: string.Empty);
152152

153153
foreach (var owner in model.Package.Owners)
154154
{
155-
_messageService.SendPackageOwnerRequestInitiatedNotice(model.CurrentUser, owner, model.User, model.Package, cancellationUrl);
155+
await _messageService.SendPackageOwnerRequestInitiatedNoticeAsync(model.CurrentUser, owner, model.User, model.Package, cancellationUrl);
156156
}
157157
}
158158

@@ -190,12 +190,12 @@ public async Task<JsonResult> RemovePackageOwner(string id, string username)
190190
throw new InvalidOperationException("You can't remove the only owner from a package.");
191191
}
192192
await _packageOwnershipManagementService.RemovePackageOwnerAsync(model.Package, model.CurrentUser, model.User, commitAsTransaction:true);
193-
_messageService.SendPackageOwnerRemovedNotice(model.CurrentUser, model.User, model.Package);
193+
await _messageService.SendPackageOwnerRemovedNoticeAsync(model.CurrentUser, model.User, model.Package);
194194
}
195195
else
196196
{
197197
await _packageOwnershipManagementService.DeletePackageOwnershipRequestAsync(model.Package, model.User);
198-
_messageService.SendPackageOwnerRequestCancellationNotice(model.CurrentUser, model.User, model.Package);
198+
await _messageService.SendPackageOwnerRequestCancellationNoticeAsync(model.CurrentUser, model.User, model.Package);
199199
}
200200

201201
return Json(new { success = true });

0 commit comments

Comments
 (0)