Do not blowup notificaiton pipeline when on notification throws

NzbDrone.Core.Test/NotificationTests/NotificationServiceFixture.cs

@@ -1,14 +1,20 @@
 using System.Collections.Generic;
 using System.Linq;
+using System.Net.Sockets;
+using FizzWare.NBuilder;
 using FluentAssertions;
+using Moq;
 using NUnit.Framework;
 using NzbDrone.Common.Composition;
+using NzbDrone.Core.MediaFiles.Events;
 using NzbDrone.Core.Notifications;
 using NzbDrone.Core.Notifications.Email;
 using NzbDrone.Core.Notifications.Growl;
 using NzbDrone.Core.Notifications.Plex;
 using NzbDrone.Core.Notifications.Prowl;
+using NzbDrone.Core.Parser.Model;
 using NzbDrone.Core.Test.Framework;
+using NzbDrone.Core.Tv;
 
 namespace NzbDrone.Core.Test.NotificationTests
 {
@@ -72,5 +78,46 @@ public void should_be_able_to_get_schema_for_all_notifications()
             notifications.Select(c => c.Instance).Should().OnlyHaveUniqueItems();
             notifications.Select(c => c.Id).Should().OnlyHaveUniqueItems();
         }
+
+        [Test]
+        [Explicit]
+        public void should_try_other_notifiers_when_one_fails()
+        {
+            var notifications = Builder<NotificationDefinition>.CreateListOfSize(2)
+                .All()
+                .With(n => n.OnGrab = true)
+                .With(n => n.OnDownload = true)
+                .TheFirst(1)
+                .With(n => n.Implementation = "Xbmc")
+                .TheLast(1)
+                .With(n => n.Implementation = "Email")
+                .Build()
+                .ToList();
+
+            var series = Builder<Series>.CreateNew()
+                .With(s => s.SeriesType = SeriesTypes.Standard)
+                .Build();
+
+            var parsedEpisodeInfo = Builder<ParsedEpisodeInfo>.CreateNew()
+                .With(p => p.EpisodeNumbers = new int[] {1})
+                .Build();
+
+            Mocker.GetMock<INotificationRepository>()
+                .Setup(s => s.All())
+                .Returns(notifications);
+
+            //Todo: How can we test this, right now without an empty constructor it won't work
+            Mocker.GetMock<Notifications.Xbmc.Xbmc>()
+                .Setup(s => s.OnDownload(It.IsAny<string>(), series))
+                .Throws(new SocketException());
+
+            Subject.Handle(new EpisodeDownloadedEvent(parsedEpisodeInfo, series));
+
+            Mocker.GetMock<Notifications.Xbmc.Xbmc>()
+                .Verify(v => v.OnDownload(It.IsAny<string>(), series), Times.Once());
+
+            Mocker.GetMock<Email>()
+                .Verify(v => v.OnDownload(It.IsAny<string>(), series), Times.Once());
+        }
     }
 }

NzbDrone.Core/Notifications/NotificationService.cs

@@ -154,34 +154,52 @@ public void Handle(EpisodeGrabbedEvent message)
         {
             var messageBody = GetMessage(message.Episode.ParsedEpisodeInfo, message.Episode.Series);
 
-            All().Where(n => n.OnGrab)
+            foreach (var notification in All().Where(n => n.OnGrab))
-                .ToList()
+            {
-                .ForEach(notification =>
+                try
-                            notification.Instance
+                {
-                                        .OnGrab(messageBody)
+                    notification.Instance.OnGrab(messageBody);
-                        );
+                }
+
+                catch (Exception ex)
+                {
+                    _logger.WarnException("Unable to send OnGrab notification to: " + notification.Name, ex);
+                }
+            }
         }
 
         public void Handle(EpisodeDownloadedEvent message)
         {
             var messageBody = GetMessage(message.ParsedEpisodeInfo, message.Series);
 
-            All().Where(n => n.OnDownload)
+            foreach (var notification in All().Where(n => n.OnDownload))
-                .ToList()
+            {
-                .ForEach(notification =>
+                try
-                            notification.Instance
+                {
-                                        .OnDownload(messageBody, message.Series)
+                    notification.Instance.OnDownload(messageBody, message.Series);
-                        );
+                }
+
+                catch (Exception ex)
+                {
+                    _logger.WarnException("Unable to send OnDownload notification to: " + notification.Name, ex);
+                }
+            }
         }
 
         public void Handle(SeriesRenamedEvent message)
         {
-            All().Where(n => n.OnDownload)
+            foreach (var notification in All().Where(n => n.OnDownload))
-                .ToList()
+            {
-                .ForEach(notification =>
+                try
-                            notification.Instance
+                {
-                                        .AfterRename(message.Series)
+                    notification.Instance.AfterRename(message.Series);
-                        );
+                }
+
+                catch (Exception ex)
+                {
+                    _logger.WarnException("Unable to send AfterRename notification to: " + notification.Name, ex);
+                }
+            }
         }
     }
 }