Improve email sending

src/NuGetGallery.Core/Services/CoreMessageService.cs

@@ -2,10 +2,12 @@
 // 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.Globalization;
 using System.Linq;
 using System.Net.Mail;
 using System.Text;
+using System.Threading;
 using AnglicanGeek.MarkdownMailer;
 using NuGet.Services.Validation;
 using NuGet.Services.Validation.Issues;
@@ -14,9 +16,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)
         {
@@ -48,7 +52,7 @@ [change your email notification settings]({emailSettingsUrl}).
 
                 if (mailMessage.To.Any())
                 {
-                    SendMessage(mailMessage, copySender: false);
+                    SendMessage(mailMessage);
                 }
             }
         }
@@ -94,7 +98,7 @@ public void SendPackageValidationFailedNotice(Package package, PackageValidation
 
                 if (mailMessage.To.Any())
                 {
-                    SendMessage(mailMessage, copySender: false);
+                    SendMessage(mailMessage);
                 }
             }
         }
@@ -162,7 +166,7 @@ public void SendValidationTakingTooLongNotice(Package package, string packageUrl
 
                 if (mailMessage.To.Any())
                 {
-                    SendMessage(mailMessage, copySender: false);
+                    SendMessage(mailMessage);
                 }
             }
         }
@@ -192,31 +196,53 @@ protected static void AddOwnersSubscribedToPackagePushedNotification(PackageRegi
             }
         }
 
-        protected void SendMessage(MailMessage mailMessage)
+        protected virtual void SendMessage(MailMessage mailMessage)
         {
-            SendMessage(mailMessage, copySender: false);
+            int attempt = 0;
+            bool success = false;
+            while (!success)
+            {
+                try
+                {
+                    AttemptSendMessage(mailMessage);
+                    success = true;
+                }
+                catch (SmtpException)
+                {
+                    if (attempt < RetryDelays.Count)
+                    {
+                        Thread.Sleep(RetryDelays[attempt]);
+                        attempt++;
+                    }
+                    else
+                    {
+                        throw;
+                    }
+                }
+            }
         }
 
-        virtual protected void SendMessage(MailMessage mailMessage, bool copySender)
+        protected virtual void AttemptSendMessage(MailMessage mailMessage)
         {
             MailSender.Send(mailMessage);
-            if (copySender)
+        }
+
+        protected void SendMessageToSender(MailMessage mailMessage)
+        {
+            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.ReplyToList.Add(mailMessage.ReplyToList.First());
-                MailSender.Send(senderCopy);
-            }
+                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.ReplyToList.Add(mailMessage.ReplyToList.First());
+            SendMessage(senderCopy);
         }
     }
 }

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
@@ -338,7 +339,7 @@ protected override void Load(ContainerBuilder builder)
                 .As<IMailSender>()
                 .InstancePerLifetimeScope();
 
-            builder.RegisterType<MessageService>()
+            builder.RegisterType<BackgroundMessageService>()
                 .AsSelf()
                 .As<IMessageService>()
                 .InstancePerLifetimeScope();

src/NuGetGallery/NuGetGallery.csproj

@@ -361,6 +361,7 @@
     <Compile Include="Services\ActionRequiringReservedNamespacePermissions.cs" />
     <Compile Include="Services\ActionsRequiringPermissions.cs" />
     <Compile Include="Services\AsynchronousPackageValidationInitiator.cs" />
+    <Compile Include="Services\BackgroundMessageService.cs" />
     <Compile Include="Services\CertificatesConfiguration.cs" />
     <Compile Include="Services\CertificateService.cs" />
     <Compile Include="Services\CertificateValidator.cs" />

src/NuGetGallery/Services/BackgroundMessageService.cs

@@ -0,0 +1,37 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Net.Mail;
+using System.Threading.Tasks;
+using AnglicanGeek.MarkdownMailer;
+using NuGetGallery.Configuration;
+
+namespace NuGetGallery.Services
+{
+    public class BackgroundMessageService : MessageService
+    {
+        public BackgroundMessageService(IMailSender mailSender, IAppConfiguration config, ITelemetryClient telemetryClient)
+            :base(mailSender, config, telemetryClient)
+        {
+        }
+
+        protected override void SendMessage(MailMessage mailMessage)
+        {
+            // Send email as background task, as we don't want to delay the HTTP response.
+            // Particularly when sending email fails and needs to be retried with a delay.
+            Task.Run(() =>
+            {
+                try
+                {
+                    base.SendMessage(mailMessage);
+                }
+                catch (Exception ex)
+                {
+                    // Log but swallow the exception.
+                    QuietLog.LogHandledException(ex);
+                }
+            });
+        }
+    }
+}

src/NuGetGallery/Services/ITelemetryClient.cs

@@ -14,5 +14,14 @@ public interface ITelemetryClient
         void TrackMetric(string metricName, double value, IDictionary<string, string> properties = null);
 
         void TrackException(Exception exception, IDictionary<string, string> properties = null, IDictionary<string, double> metrics = null);
+
+        void TrackDependency(string dependencyTypeName,
+                             string target,
+                             string dependencyName,
+                             string data,
+                             DateTimeOffset startTime,
+                             TimeSpan duration,
+                             string resultCode,
+                             bool success);
     }
 }

src/NuGetGallery/Services/MessageService.cs

@@ -3,10 +3,12 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Globalization;
 using System.Linq;
 using System.Net.Mail;
 using System.Text;
+using System.Threading.Tasks;
 using System.Web;
 using AnglicanGeek.MarkdownMailer;
 using NuGetGallery.Configuration;
@@ -16,15 +18,16 @@ namespace NuGetGallery
 {
     public class MessageService : CoreMessageService, IMessageService
     {
-        protected MessageService()
-        {
-        }
-
-        public MessageService(IMailSender mailSender, IAppConfiguration config)
+        public MessageService(IMailSender mailSender, IAppConfiguration config, ITelemetryClient telemetryClient)
             : base(mailSender, config)
         {
+            this.telemetryClient = telemetryClient ?? throw new ArgumentNullException(nameof(telemetryClient));
+            smtpUri = config.SmtpUri?.Host ?? throw new ArgumentNullException(nameof(config) + "." + nameof(config.SmtpUri));
         }
 
+        private readonly ITelemetryClient telemetryClient;
+        private readonly string smtpUri;
+
         public IAppConfiguration Config
         {
             get { return (IAppConfiguration)CoreConfiguration; }
@@ -168,7 +171,11 @@ [change your email notification settings]({7}).
 
                 if (mailMessage.To.Any())
                 {
-                    SendMessage(mailMessage, copySender);
+                    SendMessage(mailMessage);
+                    if (copySender)
+                    {
+                        SendMessageToSender(mailMessage);
+                    }
                 }
             }
         }
@@ -1014,21 +1021,20 @@ private bool AddAddressesForAccountManagementToEmail(MailMessage mailMessage, Us
             return AddAddressesWithPermissionToEmail(mailMessage, user, ActionsRequiringPermissions.ManageAccount);
         }
 
-        protected override void SendMessage(MailMessage mailMessage, bool copySender)
+        protected override void AttemptSendMessage(MailMessage mailMessage)
         {
+            bool success = false;
+            DateTimeOffset now = DateTimeOffset.UtcNow;
+            Stopwatch sw = Stopwatch.StartNew();
             try
             {
-                base.SendMessage(mailMessage, copySender);
-            }
-            catch (InvalidOperationException ex)
-            {
-                // Log but swallow the exception
-                QuietLog.LogHandledException(ex);
+                base.AttemptSendMessage(mailMessage);
+                success = true;
             }
-            catch (SmtpException ex)
+            finally
             {
-                // Log but swallow the exception
-                QuietLog.LogHandledException(ex);
+                sw.Stop();
+                telemetryClient.TrackDependency("SMTP", smtpUri, "SendMessage", null, now, sw.Elapsed, null, success);
             }
         }
 

src/NuGetGallery/Services/TelemetryClientWrapper.cs

@@ -52,5 +52,24 @@ public void TrackMetric(string metricName, double value, IDictionary<string, str
                 // logging failed, don't allow exception to escape
             }
         }
+
+        public void TrackDependency(string dependencyTypeName,
+                                    string target,
+                                    string dependencyName,
+                                    string data,
+                                    DateTimeOffset startTime,
+                                    TimeSpan duration,
+                                    string resultCode,
+                                    bool success)
+        {
+            try
+            {
+                UnderlyingClient.TrackDependency(dependencyTypeName, target, dependencyName, data, startTime, duration, resultCode, success);
+            }
+            catch
+            {
+                // logging failed, don't allow exception to escape
+            }
+        }
     }
 }

tests/NuGetGallery.Facts/Services/MessageServiceFacts.cs

@@ -2005,19 +2005,20 @@ public class TestableMessageService
             : MessageService
         {
             private TestableMessageService(IGalleryConfigurationService configurationService)
+                : base(new TestMailSender(), configurationService.Current, new Mock<ITelemetryClient>().Object)
             {
                 configurationService.Current.GalleryOwner = TestGalleryOwner;
                 configurationService.Current.GalleryNoReplyAddress = TestGalleryNoReplyAddress;
 
-                Config = configurationService.Current;
-                MailSender = MockMailSender = new TestMailSender();
+                MockMailSender = (TestMailSender)MailSender;
             }
 
             public Mock<AuthenticationService> MockAuthService { get; protected set; }
             public TestMailSender MockMailSender { get; protected set; }
 
             public static TestableMessageService Create(IGalleryConfigurationService configurationService)
             {
+                configurationService.Current.SmtpUri = new Uri("smtp://fake.mail.server");
                 return new TestableMessageService(configurationService);
             }
         }