Unintentional parallel minions

Question

If the workload of a minion takes longer time than it's WakeupInterval, in order to avoid getting a second copy of the same minion processing the same list, is there a way to know if there is a previous process (from last time the minion got invoked) is still running so the first one can skip this wakeup?

Answer 1

Update 2019-04 - Commerce 9.1 resolves the reentrancy problems, just override Minion.Execute instead of Minion.Run. For a deep dive into the changes, I've written up an article Reentrancy improvements to Minions in Sitecore Commerce 9.1

Original answer (Commerce 9.0):

First up, I consider the current behavior a bug since it directly conflicts with minions' primary usecase which is to process items from a queue. As such, I expect to it be fixed in a future version of Commerce so you might want to go with option 2 below.

Having said that, there are two ways you can go about solving this:

(note: all code is theoretical and untested - let me know if you run into any issues)

Option 1: Skip reentrant calls

This isn't difficult to implement naively, and since minions will typically wait a certain amount of time before restarting you shouldn't have to worry about multithreaded access. Having said that, minions without a WakeupInterval run the risk of complicating this situation if their execution is delayed by the OS (or TPL).

If you want to deal with potential threading issues, the typical avenues are mostly off limits since locks are per thread and minions are started using Task.Run with ConfigureAwait(false) so awaits will almost certainly complete on a different thread than they started on. There are libraries that can handle this, but that seems overkill for this one-off usage.

With all that in mind, here's a base class that uses Interlocked.CompareExchange to safely skip reentrant calls:

public abstract class NonReentrantMinion
{
    private int isRunning = 0;

    public abstract Task<MinionRunResultsModel> SafeRun();

    public override async Task<MinionRunResultsModel> Run()
    {
        if (Interlocked.CompareExchange(ref isRunning, 1, 0) != 0)
        {
            // Log skipped invocation
            return;
        }

        try
        {
            return await SafeRun();
        }
        finally
        {
            isRunning = 0;
        }
    }
}

NOTE: isRunning is intentionally non-static since Minions are singletons per Policy, which is likely inline with how you'd want the reentrancy to work.

Option 2: Queue the WakeupInterval delay

Since most minions continue to drain their managed list until it's empty, making it essentially pointless to invoke it again immediately afterwards, I consider this option to be the "fix Minion.Start" approach.

public abstract class SerialMinion
{
    public override void Start()
    {
        Task.Run(async () => 
        {
            while (true)
            {
                try
                {
                    await this.Run();
                }
                catch
                {
                    // Prevent exceptions from breaking the loop
                    // (not logged since the base implementation doesn't either)
                }

                if (this.Policy.WakeupInterval.HasValue)
                {
                    await Task.Delay(this.Policy.WakeupInterval.Value);
                }
            }
        }).ConfigureAwait(false);
    }
}