Parallel locking construct

Question

I needed an object I can use to serialize specific operations in an otherwise parallel workflow.

The specific use case I was solving for is request idempotency; a great example would be double-clicking a "Process Payment" button on a web form. I might have 100 payment processing requests in flight (parallel process) at any given time, but if two or three come in with the same credit card info, I want to make sure I process them serially (so I can deduplicate, etc.). In that case, a suitable key value might be the credit card number.

I came up with the Keybox class below. (Serializer is so overloaded...)

public class Keybox : IKeybox
{
    private readonly HashSet<string> Keys = new HashSet<string>();

    public void Lock(string key, int timeout = 60, int tick = 100)
    {
        //timeout is specified in seconds, tick in milliseconds
        //timeout = ConvertTimeoutToTicks(timeout, tick);
        //This logic is not important for the question
        timeout = 600;

        var @event = new ManualResetEvent(false);
        var counter = 0;
        do
        {
            @event.Reset();

            if (!Keys.Contains(key))
            {
                lock (Keys)
                {
                    if (!Keys.Contains(key))
                    {
                        Keys.Add(key);
                        return;
                    }
                }
            }

            Task.Delay(TimeSpan.FromMilliseconds(tick))
                .ContinueWith(t => @event.Set());

            @event.WaitOne();
        } while (counter++ < timeout);

        throw new Exception($"Unable to process '{key}'; timeout expired.");
    }

    public void Unlock(string key)
    {
        Keys.Remove(key);
    }
}

Test code:

public void Test()
{
    var box = new Keybox();
    var key = "EFF82671-DE8F-46B9-B1F1-0FB8308052C3";

    Parallel.ForEach(
        Enumerable.Range(1, 3),
        (i) =>
        {
            box.Lock(key);
            Console.WriteLine($"{i}: Running");
            Thread.Sleep(3000);
            Console.WriteLine($"{i}: Ending");
            box.Unlock(key);
        }
    );
}

which produces output like

1: Running
1: Ending
3: Running
3: Ending
2: Running
2: Ending

The main question is: is this an implementation of a specific design pattern? And the follow-on to that question is: is there a BCL class that already offers this functionality?

I'm also interested in general code review feedback.

Answer 1

    public void Lock(string key, int timeout = 60, int tick = 100)
    {
        ...
        //timeout = ConvertTimeoutToTicks(timeout, tick);
        ...
        timeout = 600;

Looks like you didn't finish tidying up the code.

---

        //timeout is specified in seconds, tick in milliseconds

Why perpetuate the common mistake in the standard libraries? TimeSpan exists: use it! I wish Microsoft would [Obsolete] every method in .Net which takes an int or a long for a timespan and replace it with a version taking a TimeSpan.

---

        var counter = 0;
        do
        {
            ...
        } while (counter++ < timeout);

Ok, now I'm even more confused. Is timeout supposed to be a timeout or a multiplier of tick?

---

            if (!Keys.Contains(key))
            {
                lock (Keys)
                {
                    if (!Keys.Contains(key))
                    {
                        Keys.Add(key);
                        return;
                    }
                }
            }

Just no. .Net's memory model fixes the subtle double-checked locking problem in Java, but only where the test is a simple boolean field. HashSet cannot guarantee that multi-threaded access will work correctly if one of the threads modifies it, so the unlocked test must be removed. However, I can offer an improvement which should compensate: use the return value of Add:

            lock (Keys)
            {
                if (Keys.Add(key))
                {
                    return;
                }
            }

Similarly, Unlock should wrap the current body in a lock statement.

---

What's the lifecycle of that ManualResetEvent? It's IDisposable, so you should take care of tidying it up yourself, probably with a using statement.

Answer 2

        Task.Delay(TimeSpan.FromMilliseconds(tick))
            .ContinueWith(t => @event.Set());

        @event.WaitOne();

I may be missing something, but how is this different from Thread.Sleep? And why would you want to pay your worker to sleep? Looks extremely inefficient in most cases.

And surely there are other names you can use apart from @event? So you can both avoid using @ and give a variable a much more meaningful name. "Event" does not tell much.