Should I use ManualResetEvent as a lock object?

Question

The method below should return true for the first call, and false for any other call.

Is there any problem with it? Is it safe to use the reset event for locking?

private ManualResetEvent _resetEvent = new ManualResetEvent(false);

public bool AmIFirst()
{
    lock (_resetEvent)
    {
        bool first = !_resetEvent.WaitOne(0);
        if (first)
            _resetEvent.Set();

        return first;
    }
}

---

Edit: I made some changes after reviewing you're remarks. I was stuck on ManualResetEvent due to former design idea. I actually don't need it at all.

class ActionSynchronizer
{
    private Timer _expirationTimer;
    private object _locker = new object();
    private bool _executionRequired = true;

    private SomeDelegate _onExpired = delegate { };

    public ActionSynchronizer(SomeDelegate onExpired)
    {
        _onExpired = onExpired;
        expirationTimer = new Timer(OnExpired, null, 30000, Timeout.Infinite);
    }

    public bool IsExecutionRequired()
    {
        if (!_executionRequired)
            return false;

        lock (_locker)
        {
            if (_executionRequired)
            {
                _executionRequired = false;
                return true;
            }

            return false;
        }
    }

    private void OnExpired(object state)
    {
        if (_executionRequired)
        {
            lock (_locker)
            {
                if (_executionRequired)
                {
                    _executionRequired = false;
                    // https://mcmap.net/q/67163/-why-does-asynchronous-delegate-method-require-calling-endinvoke/1712747#1712747
                    _onExpired.BeginInvoke(_originalAction, EndInvoke, null);
                }
            }
        }
    }
}

// ...
{
    if (_action.Sync.IsExecutionRequired())
        _action.Invoke();
}

Triplex

Answer 1

I would go a different route here...

private int counter;
...
if(Interlocked.Increment(ref counter) == 1)
{
     // yes, I'm first
}

Thread safe, no locks. Or if you are worried about wrapping around Int32:

if(Interlocked.CompareExchange(ref counter, 1, 0) == 0)
{
     // yes, I'm first
}   

Descent

+1 for this - I like - I was gong to suggest Interlocked, but didn't know about CompareExchange - thanks! – Hydrogenous

Thanks @Marc. If you find the time, please review my edit. Do you think Interlocked is still an appropriate solution? – Triplex

@Triplex as long as you use an Interlocked.CompareExchange(ref whatever, 0, 1) == 1 in OnExpired, you should be fine. That says "if it is 1, change it back to 0 and do the work", but is thread-safe etc – Descent

I love lock-free tricks! Thank you for teaching me a new one. – Triplex

Answer 2

Nowadays, I only ever lock() on a simple System.Object object which I've created just for locking with.

I definitely wouldn't lock() on something like an Event, not because it wouldn't work, but because I think it's potentially rather confusing to be using lock() on an object which it is itself (though completely separately) associated with kernel locking type operations.

I'm not clear what you're actually doing here, but it looks rather like something which a named Mutex might do better.

Marillin

Answer 3

I think it's better to use lock() for this on an object.

Also, you can prevent excess thread locking by using a "double-checked locking"

e.g.

private object _protection = new object();
private bool _firstTime = true;

public bool AmIFirst()
{
    if (!_firstTime)
        return false;

    lock (_protection)
    {
        if (!_firstTime)
            return false;

        _firstTime = false;
        return true;
    }
}

---

Note... - there's some interesting comments on double-checked locking - Double-checked locking in .NET - I'm still reading up on this!

---

Another note... its not clear from the code snippet you posted, but if you are looking to implement a global singleton then solution 4 on http://www.yoda.arachsys.com/csharp/singleton.html is a good place to start

Hydrogenous

+1 for double check. It's not a singleton, it's more similar to load balancer. I queue this object in some queues and on dequeue if (action.AmIFirst()) action.Invoke();. I'm also running a timer for expiration. If timer ticks before execution, some other delegate is called. – Triplex

Thanks - I can't really argue with Jon though - "Personally, if I'm in a situation where I have to choose between double-checked locking and "lock every time" code I'd go for locking every time until I'd got real evidence that it was causing a bottleneck." from his answer to #395398 - so unless you know this is a bottleneck, then maybe go for the simple safe single lock? – Hydrogenous

Answer 4

The only thing you need to make sure is that the same object you lock on is accessible to all instances of the code that needs synchronizing. Other than that, no problem.

Undertow

there's a second part needed though... "and is not used in a Monitor for any other purpose"; it is hard to make this guarantee, especially when the compiler itself used to (until 4.0) use lock(this) for synchronizing event subscriptions... any class with an event, that has been compiled on a non-4.0 compiler is a potential hazard. I'm not saying it is necessarily a problem - but just you need to be aware of it. – Descent