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.

• Disappointed: Does in flight here means parallel ?
• Matt Mills: @Disappointed in flight here means "currently being processed". english.stackexchange.com/a/189987/25517

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.

• Matt Mills: +1. Fantastic advice about using TimeSpan instead of int or long. FWIW, the commented lines were indeed tidied up, per this site's admonition to "post working code samples" where possible.
• Matt Mills: This is the first time I've worked with WaitHandles, and I thought about checking whether it was IDisposable after I put the code up for review, but didn't get back to it.

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.

• Matt Mills: There's one important difference between Thread.Sleep() and Task.Delay() - with Task.Delay(), no threads are being blocked while the timeout is awaited. Otherwise, they are behaviorally similar, especially since I'm not using a CancellationToken.
• Matt Mills: I agree that @event is not the greatest name - that's a leftover from the proof-of-concept phase.
• Nikita B: @arootbeer yes, but event.WaitOne(); is a blocking call so it completely negates any advantages gained by asynchronous nature of Task.Delay.
• Matt Mills: Heh - the things we take for granted. Looks like I'll be doing some more reading. (Starting here: hanselman.com/blog/…. If you've got suggestions, bring 'em on!)