Long running in-process Publish/Subscribe class

Question

I have a Pub/Sub class that will call Notify() when published and that Notify() could take around 10 seconds, so I need to make sure that this does not block the UI.

public static class NotificationService
{
 private static Dictionary<string, INotification> Observers { get; }
 static NotificationService()
 {
 Observers = new Dictionary<string, INotification>();
 }
 public static void Publish(string title, string requester)
 {
 foreach (var observer in Observers)
 {
 var notification = observer.Value;
 new Thread(() =>
 {
 Thread.CurrentThread.IsBackground = true;
 notification.Notify(title, requester);
 }).Start();
 }
 }
 public static void Subscribe(INotification notification)
 {
 INotification notificationValue;
 if (Observers.TryGetValue(notification.NotificationName, out notificationValue))
 {
 // Observer already exists
 return;
 }
 Observers[notification.NotificationName] = notification;
 }
 public static void UnSubscribe(INotification notification)
 {
 INotification notificationValue;
 if (!Observers.TryGetValue(notification.NotificationName, out notificationValue))
 {
 // Observer doesn't exists
 return;
 }
 Observers.Remove(notification.NotificationName);
 }
}

INotification

public interface INotification
{
 string NotificationName { get; }
 bool Notify(string title, string requester);
}

Example implementation of INotification

public class EmailMessageNotification : INotification
{
 public EmailMessageNotification(EmailNotificationSettingsService settings)
 {
 EmailNotificationSettings = settings;
 }
 private EmailNotificationSettingsService EmailNotificationSettings { get; }
 public string NotificationName => "EmailMessageNotification";
 public bool Notify(string title, string requester)
 {
 var configuration = GetConfiguration();
 if (!ValidateConfiguration(configuration))
 {
 return false;
 }
 var message = new MailMessage
 {
 IsBodyHtml = true,
 To = { new MailAddress(configuration.RecipientEmail) },
 Body = $"User {requester} has requested {title}!",
 From = new MailAddress(configuration.EmailUsername),
 Subject = $"New Request for {title}!"
 };
 try
 {
 using (var smtp = new SmtpClient(configuration.EmailHost, configuration.EmailPort))
 {
 smtp.Credentials = new NetworkCredential(configuration.EmailUsername, configuration.EmailPassword);
 smtp.EnableSsl = configuration.Ssl;
 smtp.Send(message);
 return true;
 }
 }
 catch (SmtpException smtp)
 {
 Log.Fatal(smtp);
 }
 catch (Exception e)
 {
 Log.Fatal(e);
 }
 return false;
 }
 private EmailNotificationSettings GetConfiguration()
 {
 // Gets and Returns Config
 }
 private bool ValidateConfiguration(EmailNotificationSettings settings)
 {
 // Do some validation
 }

Can anyone see anything wrong with this?

Answer 1

1. It seems that you have a global static logger methods. It is not a good practice, as you're loosing the possibility to tune logging levels for different areas of your application (e.g. setting log level for EmailMessageNotification to DEBUG while keeping INFO level everywhere else. I recommend to follow the logging patterns suggested for your logging framework (see Creating loggers for NLog for example).
2. The code is not thread-safe, so you may get issues if your application publishes, subscribes or unsubscribes in different threads. Use either locking around Observers collection or ‎ConcurrentDictionary.
3. Fatal logging level should be used only when the application encounters a critical error after which it usually cannot continue to run. In your case I would suggest to use Error log level when email cannot be sent.
4. NotificationService is static, which means that you will have a strong dependencies in your code on this class. I suggest to introduce IoC framework (check out Autofac or StructureMap).
5. The response of INotification.Notify(string, string) is not used, so it can be void.
6. Your current code requires spinning up threads per each observer per each notification. I suggest to switch to asynchronous processing model, changing the return type of INotification.Notify(string, string) to Task.
7. Most likely you would want to wrap the notification of each listener in try..catch to avoid side effects when buggy INotification subscribes to your service

As a result:

INotification:

public interface INotification
{
 string NotificationName { get; }
 Task NotifyAsync(string title, string requester);
}

EmailMessageNotification:

public class EmailMessageNotification : INotification
{
 private static Logger logger = LogManager.GetCurrentClassLogger();
 public EmailMessageNotification(EmailNotificationSettingsService settings)
 {
 EmailNotificationSettings = settings;
 }
 private EmailNotificationSettingsService EmailNotificationSettings { get; }
 public string NotificationName => "EmailMessageNotification";
 public async Task NotifyAsync(string title, string requester)
 {
 var configuration = GetConfiguration();
 if (!ValidateConfiguration(configuration))
 {
 return;
 }
 var message = new MailMessage
 {
 IsBodyHtml = true,
 To = { new MailAddress(configuration.RecipientEmail) },
 Body = $"User {requester} has requested {title}!",
 From = new MailAddress(configuration.EmailUsername),
 Subject = $"New Request for {title}!"
 };
 try
 {
 using (var smtp = new SmtpClient(configuration.EmailHost, configuration.EmailPort))
 {
 smtp.Credentials = new NetworkCredential(configuration.EmailUsername, configuration.EmailPassword);
 smtp.EnableSsl = configuration.Ssl;
 await smtp.SendMailAsync(message).ConfigureAwait(false);
 }
 }
 catch (SmtpException smtp)
 {
 logger.Error(smtp);
 }
 catch (Exception e)
 {
 logger.Error(e);
 }
 }
 private EmailNotificationSettings GetConfiguration()
 {
 throw new NotImplementedException();
 }
 private bool ValidateConfiguration(EmailNotificationSettings settings)
 {
 throw new NotImplementedException();
 }
}

NotificationService:

public interface INotificationService
{
 void Publish(string title, string requester);
 void Subscribe(INotification notification);
 void UnSubscribe(INotification notification);
}
public class NotificationService : INotificationService
{
 private static Logger logger = LogManager.GetCurrentClassLogger();
 private ConcurrentDictionary<string, INotification> Observers { get; } = new ConcurrentDictionary<string, INotification>();
 private static async Task NotifyAsync(INotification notification, string title, string requester)
 {
 try
 {
 await notification.NotifyAsync(title, requester).ConfigureAwait(false);
 }
 catch (Exception ex)
 {
 logger.Error(ex, $"Notification '{notification.NotificationName}' failed with exception");
 }
 }
 public async void Publish(string title, string requester)
 {
 IEnumerable<Task> notificationTasks = Observers.Values.Select(notification => NotifyAsync(notification, title, requester));
 await Task.WhenAll(notificationTasks).ConfigureAwait(false);
 }
 public void Subscribe(INotification notification)
 {
 Observers.TryAdd(notification.NotificationName, notification);
 }
 public void UnSubscribe(INotification notification)
 {
 Observers.TryRemove(notification.NotificationName, out notification);
 }
}

• Jamie Rees: Regarding the NotificationService being static, this is because I am subscribing observers from different places in the Application. So If I wanted to Pusblish to all observers I would need to new() and then resubscribe all observers? Regarding the loggers, Looks like I removed them from the original question (I trimmed some usless stuff out). They are private static per class and not something global.
• almaz: IoC framework will allow you to use NotificationService as a dependency (receive the instance of INotificationService in the constructor) rather than instantiate it yourselves. I would recommend you to look deeper into Dependency Injection pattern
• Jamie Rees: I am currently using an IOC container with constructor injection, so get the instance of the service (Like a singleton) via DI?
• almaz: yes, wherever you need an instance of INotificationService- just inject it in constructor. At the registration time you should specify that only one instance of the NotificationService should be created (SingleInstance in Autofac, .Singleton() in StructureMap)