Is this dependency of a Windsor singleton thread-safe?

Question

I'm not an expert on async programming by any means so I want to verify I have an issue.

I have a Web API app that uses Castle Windsor but also uses the built-in HttpConfiguration.Services pipeline for certain ASP.NET functions. In this case I'm registering a global exception handler. Here's the code:

protected void Application_Start()
{
    //ASP.NET registers this instance in a ConcurrentDictionary and treats it as a singleton
    config.Services.Replace(typeof(IExceptionHandler), container.Resolve<IExceptionHandler>()); 
}

public class EmailExceptionHandler : ExceptionHandler
{
    private readonly SmtpClient client;
    private MailMessage _errorMail;

    public EmailSender(SmtpClient client, MailMessage message) 
        //client automatically resolved with smtp settings pulled from web.config by container. Seems okay to be a singleton here.
        //message automatically resolved with properties like To, From populated from web.config.
        //The intent here is to keep config access out of this class for testability.
    {
        _errorSmtpClient = errorSmtpClient;
        _errorMail = errorMail;
    }

    public override void Handle(ExceptionHandlerContext context)
    {
        // set props on the MailMessage e.g. exception detail

        _errorSmtpClient.SendAsync(_errorMail);

        // standard post-processing, no dependencies necessary
    }
}

public void Install(IWindsorContainer container, IConfigurationStore store)
{
    container.Register(Component.For<SmtpClient>().DependsOn(Dependency.OnAppSettingsValue(/*...*/)));

    container.Register(Component.For<MailMessage>().Named("errorMailMessage")
        .DependsOn(Dependency.OnAppSettingsValue(/*...*/)).LifestyleTransient()); 
        //transient here should bind lifetime to exceptionhandler singleton's lifetime

    container.Register(Component.For<IExceptionHandler>().ImplementedBy<EmailExceptionHandler>()
                        .DependsOn(Dependency.OnComponent("message", "errorMailMessage")));
}

When an unhandled exception occurs, ASP.NET will go look in its Services dictionary for a registered IExceptionHandler and passes it an error context. In this case, that's the handler I've wired up in Windsor and registered at application start.

Here's the .NET Framework code that calls the Handle override I've defined:

Task IExceptionHandler.HandleAsync(ExceptionHandlerContext context, CancellationToken cancellationToken)
{
  if (context == null)
    throw new ArgumentNullException("context");
  ExceptionContext exceptionContext = context.ExceptionContext;
  if (!this.ShouldHandle(context))
    return TaskHelpers.Completed();
  return this.HandleAsync(context, cancellationToken);
}

public virtual Task HandleAsync(ExceptionHandlerContext context, CancellationToken cancellationToken)
{
  this.Handle(context);
  return TaskHelpers.Completed();
}

The MailMessage is being resolved at application start and persisted throughout the lifetime of the container due to the parent singleton never being disposed. As such, I am concerned that concurrent requests that throw exceptions will cause their respective threads to enter the code managing the MailMessage, possibly leaving it in an undesirable state.

The complexity here is that not only do I have to figure out if I have a potential issue where concurrent threads can change the state of the MailMessage, I have to make sure I solve said issue by managing the threads correctly without causing deadlocks due to the async nature of the flow.

Should the issue exist, I can think of several ways to address it:

• create a lock statement around the setting of the message and sending of the email. Since the void method itself is not async the only drawback seems to be causing concurrent threads to block until they can enter. Wouldn't this also be the same as using a SemaphoreSlim's Wait() method?

• create a Typed factory dependency and explicitly resolve an instance of the MailMessage in the Handle method, and assign it to a local variable.

• don't block other threads by using async all the way down and calling SemaphoreSlim.WaitAsync() -- will this work?

Like this:

public override async void Handle(ExceptionHandlerContext context)
{
    await _semaphoreSlim.WaitAsync();
    try
    {
        await _errorSmtpClient.SendMailAsync(_errorMail);
    }
    finally
    {
        _semaphoreSlim.Release();
    }
}

Answer 1

Neither the dependency nor the singleton itself are thread-safe.

Instance methods of SmtpClient are not thread-safe. This is confirmed in this question. Therefore your singleton, which relies upon a single SmtpClient, is not thread-safe.

Instance methods of MailMessage are also not thread-safe. So again your singleton is not thread-safe.

In addition, I can find nothing to suggest that MailMessage is intended to be reusable. The object implements IDisposable and encapsulates other objects that also implement IDisposable. It is possible for the message to hold unmanaged resources and therefore it seems logical to conclude that MailMessage is intended to be single use and should be disposed once it has been sent. See this question for further discussion.

If you wish to proceed with only a single MailMessage, which is reused, and a single SmtpClient you will need to synchronise all uses of these objects. Even then, I would expect you may still have issues with unmanaged resources not being properly released.

It seems the simplest and safest implementation of your ExceptionHandler would dynamically construct both the MailMessage and SmtpClient each time it is called, and then dispose of the MailMessage after transmission.