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;
                    // http://stackoverflow.com/questions/1712741/why-does-asynchronous-delegate-method-require-calling-endinvoke/1712747#1712747
                    _onExpired.BeginInvoke(_originalAction, EndInvoke, null);
                }
            }
        }
    }
}

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

score 15 · Accepted Answer · answered Mar 21 '11 at 09:28

15

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
}

answered Mar 21 '11 at 09:28

Marc Gravell

1,026,079
266
2,566
2,900

2

+1 for this - I like - I was gong to suggest Interlocked, but didn't know about CompareExchange - thanks! – Stuart Mar 21 '11 at 09:33
Thanks @Marc. If you find the time, please review my edit. Do you think `Interlocked` is still an appropriate solution? – HuBeZa Mar 21 '11 at 10:14
@HuBeZa 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 – Marc Gravell Mar 21 '11 at 10:44
I love lock-free tricks! Thank you for teaching me a new one. – HuBeZa Mar 21 '11 at 13:09

score 2 · Answer 2 · answered Mar 21 '11 at 09:21

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.

score 1 · Answer 3 · edited May 23 '17 at 12:09

1

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

edited May 23 '17 at 12:09

Community

1
1

answered Mar 21 '11 at 09:14

Stuart

66,722
7
114
165

+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. – HuBeZa Mar 21 '11 at 09:48
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 http://stackoverflow.com/questions/394898/double-checked-locking-in-net - so unless you know this is a bottleneck, then maybe go for the simple safe single lock? – Stuart Mar 21 '11 at 09:52

score 0 · Answer 4 · answered Mar 21 '11 at 09:16

0

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.

answered Mar 21 '11 at 09:16

Vincent Vancalbergh

3,267
2
22
25

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. – Marc Gravell Mar 21 '11 at 09:26

Should I use ManualResetEvent as a lock object?

4 Answers4