views:

32

answers:

2

Suppose I am processing a large amount of incoming data from multiple threads. I may want for this data to act as a trigger for a specific action when certain criteria are met. However, the action is not reentrant; so the same trigger being fired twice in rapid succession should only cause the action to run once.

Using a simple bool flag to indicate whether the action is currently running is not a robust solution, as a race condition could occur and two (or more) threads could still end up running the same action concurrently after all. A bool within a lock statement on a synchronization object would work, of course; but I generally prefer to avoid locking whenever possible*.

Currently what I do in these cases is use an AutoResetEvent as essentially a form of atomic switch. The code typically looks something like this:

if (conditionMet && readyForAction.WaitOne(0))
{
    try
    {
        doAction();
    }
    finally
    {
        readyForAction.Set();
    }
}

This works, but it strikes me that this may be a less-than-ideal use case for the AutoResetEvent class. Would you say this is an appropriate solution to this problem, or would it make more sense to use some other mechanism?


Update: As Jon pointed out, using Monitor.TryEnter as the locking strategy (instead of the simple lock, which I believe is roughly equivalent to Monitor.Enter), is really quite straightforward:

if (conditionMet && Monitor.TryEnter(lockObject))
{
    try
    {
        doAction();
    }
    finally
    {
        Monitor.Exit(lockObject);
    }
}

That said, I'm strongly inclined to go with Henk's idea of using Interlocked. That seems about as simple as it gets.

+2  A: 

A simple (and fast) System.Threading.Interlocked.CompareExchange() will do.

//untested
int flag = 0;

if (conditionMet && Interlocked.CompareExchange(ref flag, 1, 0) == 0)
{
    try
    {
        doAction();
    }
    finally
    {
        flag = 0; // no need for locking
    }
}
Henk Holterman
Ah, so you're saying: if it returns 1, then don't enter the code? And then call `Decrement` in the `finally`?
Dan Tao
@Dan, yes, but you will need CompareExchange, not Increment. Will edit later.
Henk Holterman
@Henk: Right, right... seems like just `Exchange` could work, actually, couldn't it?
Dan Tao
@Dan: No, you can't do an `if` with just Exchange and still avoid the race condition.
Henk Holterman
@Henk: I must be missing something... why not? Why couldn't I do `if (Interlocked.Exchange(ref flag, 1) == 0)`? It seems to me that the only difference between these approaches is that `CompareExchange` only sets `flag` to 1 if it's presently 0... but it doesn't actually matter whether or not it redundantly sets `flag` to 1; it only matters whether or not it's presently 0. Right? Am I overlooking something?
Dan Tao
@Dan, Yes you're right, I always have to think twice with this. It returns the old value so Exchange is OK. I'll leave my answer for now.
Henk Holterman
@Henk: This was a great answer. Something just occurred to me, though... it seems like `Interlocked.Increment` could've worked after all; you would just check that the return value is 0, and then set it to 0 again in the `finally`. Right? I guess that's just a variation on this same idea, actually...
Dan Tao
+1  A: 

"I generally prefer to avoid locking whenever possible" - why? It looks like you're effectively doing something very similar here, but using Windows synchronization primitives instead of CLR ones. You're trying to synchronize so that only one thread can be running a particular piece of code at a time... that certainly sounds like a lock to me. The only difference is that if the lock is already held, you don't want to bother executing the code at all.

You should be able to achieve the same effect using Monitor.TryEnter, which I would personally argue is a bit simpler to understand. (Then again, I "grew up" with Java monitors, so it's closer to my background.)

Jon Skeet
Definitely good points... my aversion to `lock` is not really an "avoid like the plague" sort of attitude, more just "go with something simpler if possible." For example using `lock` or `Monitor.TryEnter` requires a separate object for that sole purpose. It's also a bit cumbersome in this context: I'd have to do something like `bool switched; lock (m_switchLock) { if (!m_switch) { m_switch = true; switched = true; } } if (switched) { ... } m_switch = false;` Granted, that's for `lock`, not `Monitor.TryEnter`, which as you say would certainly be simpler.
Dan Tao