UnitOfWork for sending mail

Question

Topic is about a LoB web application written in C# for ASP.Net Core. We're following the blue book DDD architecture. We're using SimpleInjector as dependency injection container. Every request opens a new injection scope and implicitly a UnitOfWork and a database transaction which is committed/completed when the response leaves the business logic layer. Fine.

Now there is an application service, that requires control over the transaction and unit of work. The service is in charge of sending queued messages via SMTP, and since the SMTP communication takes some seconds for each mail, it should follow the pattern

1. Dequeue message
2. Mark as "InProgress"
3. Complete UnitOfWork
4. Do the SMTP stuff
5. Open new UnitOfWork
6. Get the message, mark it as "Succeeded"
7. Complete UnitOfWork

I am now trying to show the most important parts how I managed this to happen, but I still have a bad feeling about this. Inside my SmtpSendingService the Scope is being used to get services from the injection container. The service is obviously lying about its dependencies, but the domain service instances that could have been injected via constructor would be stale after Completing and Reopening the scope.

ExplicitScope: the instance that holds the current SimpleInjector Scope and the current IUnitOfWork instance

public class ExplicitScope : IDisposable
    {
        private readonly Container container;
        private readonly Scope scope;
        private readonly IUnitOfWork unitOfWork;

        public ExplicitScope(Container container, bool withReadonlyUnitOfWork)
        {
            this.container = container;
            scope = container.BeginExecutionContextScope();
            unitOfWork = withReadonlyUnitOfWork 
                ? container.GetInstance<IReadonlyUnitOfWork>()
                : container.GetInstance<IUnitOfWork>();
        }

        public T GetInstance<T>() where T : class
        {
            return container.GetInstance<T>();
        }

        public IEnumerable<T> GetAllInstances<T>() where T : class
        {
            return container.GetAllInstances<T>();
        }

        public void Complete()
        {
            unitOfWork.Complete();
        }

        public void Dispose()
        {
            unitOfWork?.Dispose();
            scope?.Dispose();
        }
    }

IScopeStarter is used by the middleware to open and close an ExplicitScope for every request

public interface IScopeStarter
{
    ExplicitScope BeginExplicitScope();
    ExplicitScope BeginExplicitReadonlyScope();
}

(We have a specific readonly scope that is used in GET requests. REST says a GET must never change anything, so the transaction is always rolled back in this case and the ORM-context does no change tracking, saving some bytes and milliseconds)

Runtime, basically a wrapper for the container, the wiring is done here, and it implements the IScopeStarter interface

public class Runtime : IScopeStarter
{
    private Container container { get; } = new Container();

    public void Boot()
    {
        // boring container wiring
    }

    public ExplicitScope BeginExplicitScope()
    {
        return new ExplicitScope(container, false);
    }

    public ExplicitScope BeginExplicitReadonlyScope()
    {
        return new ExplicitScope(container, true);
    }
}

SmtpSendingService: the bad guy.

public class SmtpSendingService
{
    private readonly IScopeStarter scopeStarter;
    private static readonly ILogger Logger = LogManager.Create<SmtpSendingService>();

    public SmtpSendingService(IScopeStarter scopeStarter)
    {
        this.scopeStarter = scopeStarter;
    }

    public void SendAllQueuedMessages()
    {
        Logger.Info("Beginning to send all unsent messages in the queue.");

        SmtpMessage messageToSend;
        while (TryDequeueSmtpMessage(out messageToSend))
        {
            Send(messageToSend);
        }

        Logger.Info("No more unsent messages in the queue. Terminating now.");
    }

    private void Send(SmtpMessage message)
    {
        ISmtpClient smtpClient;
        using (var scope = scopeStarter.BeginExplicitScope())
        {
            // surprise, surprise, I need an SmtpClient, but I don't tell anyone
            smtpClient = scope.GetInstance<ISmtpClient>();
        }

        // without surroundiong scope -> no open transaction during unpredictable duration of SMTP communication
        string result = smtpClient.Send(message);

        using (var scope = scopeStarter.BeginExplicitScope())
        {
            // even better, an arbitary DomainService, too, but I don't tell anyone either 
            scope.GetInstance<ISmtpMessageService>().LogResult(message.Id, result);
            scope.Complete();
        }
    }

    /// <summary>
    /// Tries to get the next unsent SMTP message from the database table 
    /// </summary>
    /// <param name="message"></param>
    /// <returns>True when an unsent message was found</returns>
    private bool TryDequeueSmtpMessage(out SmtpMessage message)
    {
        using (var scope = scopeStarter.BeginExplicitScope())
        {
            // well, yeah, you see the problem...
            bool success = scope.GetInstance<ISmtpMessageService>().TryDequeueSmtpMessage( out message);
            scope.Complete();
            return success;
        }
    }
}

So, my bad feeling was there from the beginning, but when I started to write tests for the bad guy and began thinking how to mock the container implicitly used by the ExplicitScope I started to write this question.

And for the obvious suggestion: "Don't do it" - we tried and got burnt. There are peaks in queueing and dequeuing messages that lead to deadlocks when the sending service just keeps the transaction open.

Answer 1

I don't know if this falls under "don't do it", but you shouldn't be passing your "container" around. That should be used to wire things up on startup. The rest of your classes should know nothing about it. That includes stuff like your "scope" class which calls GetInstance on the fly. They should take what they actually need (ideally through the constructor), not an IoC container which they use to resolve what they need. That's the Service Locator anti-pattern.

If the class needs to be able to get new instances, inject a factory. If the class needs an instance, inject an instance. Then you can easily test each class because you just insert a mock factory or mock instance etc.

Answer 2

So, here's what I ended up with. Just using a factory as suggested by @eurotrash is a guide in the correct direction, but won't solve the problem of stale instances after disposing and reopening the injection scope.

IScopeInterrupter is the interface that allows this special case

public interface IScopeInterruptor
{
    void CompleteCurrentScope_InvokeAction_BeginNewScope(Action action);

    T CompleteCurrentScope_InvokeFunction_BeginNewScope<T>(Func<T> func);
}

Implementor is my ExplicitScope

// ...
    public void CompleteCurrentScope_InvokeAction_BeginNewScope(Action action)
    {
        bool hasCurrentUnitOfWork = unitOfWork != null;
        if (hasCurrentUnitOfWork)
        {
            unitOfWork.Complete();
            unitOfWork.Dispose();
            unitOfWork = null;
        }
        scope.Dispose();
        scope = null;

        action.Invoke();

        scope = runtime.container.BeginExecutionContextScope();
        if (hasCurrentUnitOfWork)
        {
            BeginUnitOfWork(isReadonlyUnitOfWork);
        }
    }

    public T CompleteCurrentScope_InvokeFunction_BeginNewScope<T>(Func<T> func)
    {
        T result = default(T);
        CompleteCurrentScope_InvokeAction_BeginNewScope(() =>
        {
            result = func.Invoke();
        });
        return result;
    }
// ...

LateResolver<T>

is a wrapper around a factory method for an injected service, that is not resolved on injection time, but - well - late:

public class LateResolver<T>
{
    private readonly Func<T> factoryMethod;

    public LateResolver(Func<T> factoryMethod)
    {
        this.factoryMethod = factoryMethod;
    }
    public T Resolve()
    {
        return factoryMethod.Invoke();
    }
}

Registration is done like this:

container.Register(()=>new LateResolver<ISmtpMessageService>(() => container.GetInstance<ISmtpMessageService>()), Lifestyle.Singleton);

the previously bad guy:

Is now happily communicating all its demands via the constructor. Everyone can see that this is a special case, because of the demand for late resolution. And testing with mocks is easy.

public class SmtpSendingService
{
    private readonly ScopeInterruptorHolder scopeInterruptorHolder;
    private readonly ISmtpClient smtpClient;
    private readonly LateResolver<ISmtpMessageService> smtpMessageServiceLateResolver;
    private static readonly ILogger Logger = LogManager.Create<SmtpSendingService>();

    public SmtpSendingService(ISmtpClient smtpClient, ScopeInterruptorHolder scopeInterruptorHolder, LateResolver<ISmtpMessageService> smtpMessageServiceLateResolver)
    {
        this.scopeInterruptorHolder = scopeInterruptorHolder;
        this.smtpClient = smtpClient;
        this.smtpMessageServiceLateResolver = smtpMessageServiceLateResolver;
    }

    public void SendAllQueuedMessages()
    {
        Logger.Info("Beginning to send all unsent messages in the queue.");

        SmtpMessage messageToSend;
        ISmtpSettings smtpSettings;
        while (TryDequeueSmtpMessage(out messageToSend, out smtpSettings))
        {
            Send(messageToSend, smtpSettings);
        }

        Logger.Info("No more unsent messages in the queue. Terminating now.");
    }

    private bool TryDequeueSmtpMessage(out SmtpMessage message, out ISmtpSettings smtpSettings)
    {
        bool success = smtpMessageServiceLateResolver.Resolve().TryDequeueSmtpMessage(out message, out smtpSettings);
        return success;
    }

    private void Send(SmtpMessage message, ISmtpSettings smtpSettings)
    {
        // without surrounding scope -> no open transaction during unpredictable duration of SMTP communication
        string result = scopeInterruptorHolder.ScopeInterruptor.CompleteCurrentScope_InvokeFunction_BeginNewScope(() => smtpClient.Send(message, smtpSettings));

        //MAGIC: the instance resolved here won't be equal to the instance resolved during dequeuing of the message ;-)
        smtpMessageServiceLateResolver.Resolve().LogResult(message.Id, result);
    }
}

However, I would not suggest to promote the scope interruptor and late resolution as a first class citizen in your application layer, but if there are good reasons like in this special case, it solves a problem in an elegant and less surprising manner, I think.