4
\$\begingroup\$

I want a code run just once (say in Dispose). WriteOnceBlock<T> from TPL Data Flow could be used here; but again if we need to check if it is done (in a 'not data flow' friendly manner), we have to call Receive and timeout and things.

I wrote this and it works, but I am not sure about when comparison takes place (==); is it possible for that to be rearranged so something goes wrong and out of time/order?

public class Once
{
    const Int32 JobDone = 11011;
    const Int32 NotDone = 119;

    Int32 _done;

    public Once() { _done = NotDone; }

    public bool IsDone() { return Interlocked.CompareExchange(ref _done, JobDone, NotDone) == JobDone; }
}

And usage:

readonly Once _stopped = new Once();
public void OnStop()
{
    if (_stopped.IsDone()) return;

    //...
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I'm not sure this might work for your scenario, but you might want to look at : msdn.microsoft.com/en-ca/library/dd642331%28v=vs.110%29.aspx \$\endgroup\$ – IEatBagels Dec 17 '14 at 13:59
  • \$\begingroup\$ Are you looking for behavior like C++11's call_once? In addition to doing an operation only once, it also guarantees that the operation will have been completed before it returns (even if that means blocking for another thread which is doing the op), and it synchronizes with that thread. Doing all of those extra steps takes more work than just the things you asked for. \$\endgroup\$ – Cort Ammon Dec 17 '14 at 21:09
  • \$\begingroup\$ What you've referred to here is achievable exactly with WriteOnceBlock<T> - as I've mentioned at the first line - or Lazy<T> as @TopinFrassi suggested. Both are too much of a ceremony for just ensuring a method is already called or not. You are right about ensuring if the action is completed, which again can be done using a simple Task and continue in case of fault and such. But there are situations that this class would suffice; it is thread-safe and non-blocking. \$\endgroup\$ – Kaveh Shahbazian Dec 17 '14 at 21:49
  • \$\begingroup\$ What you can / can't do after receiving answers \$\endgroup\$ – Heslacher Dec 21 '14 at 17:18
  • \$\begingroup\$ You could implement IDisposable so that when the object is disposed you throw an Exception if it get calls after disposed, this ensure the the object can't be reuse. \$\endgroup\$ – Tien Dinh Dec 27 '14 at 17:23
10
\$\begingroup\$

There are a few items to make this code better.

  1. Naming. Once is an OK name for the class, but the method name IsDone is a problem. This is an 'atomic' operation that sets values, as well as gets values. A method called something like "Trigger", and changing the class name to a common term like OneShot, will give you the semantics like:

    private readonly OneShot terminator = new OneShot();
    
    
    if (terminator.Trigger())
    {
        ... do something if we are the first trigger
    }
    

    or, using your semantics, the negated value:

    if (!terminator.Trigger()) return;
    
  2. Your fields should all be private.

    private const Int32 JobDone = 11011;
    private const Int32 NotDone = 119;
    
    private Int32 _done;
    

    otherwise other code can possibly reset or mess up your trigger.

  3. Why use the bizarre numbers for the constants? What's wrong with 1, and 0. The special numbers make me think there's something especially magical with them.

Apart from that, the Interlocked.CompareExchange is the right tool for the job. It creates an atomic compare-and-set operation that makes it thread safe.

\$\endgroup\$
  • \$\begingroup\$ Thanks for great answer! Still I like IsDone better, it is read so to me! And fields are all private; that's the default member access in C#. \$\endgroup\$ – Kaveh Shahbazian Dec 17 '14 at 18:25
  • \$\begingroup\$ @KavehShahbazian IsDone implies that the method checks whether it is done, it doesn't say that is also changes the state to done. I would be confused by that. \$\endgroup\$ – svick Dec 18 '14 at 15:12
  • \$\begingroup\$ @svick You are right. But here is another problem: we need again to call the sequence of methods like if !IsDone then SetDone; if IsDone then return; the 2 step verification. I agree that IsDone is a bad name for my method. But it removes the need for a two step verification. What should it be called? \$\endgroup\$ – Kaveh Shahbazian Dec 19 '14 at 10:48
5
\$\begingroup\$

Spacing

Single-line methods, while possible, are pretty much universally a bad idea. It costs nothing to split them over a few lines, and you gain tons in readability.

public bool IsDone() { return Interlocked.CompareExchange(ref _done, JobDone, NotDone) == JobDone; }

looks better as:

public bool IsDone() 
{ 
    return Interlocked.CompareExchange(ref _done, JobDone, NotDone) == JobDone;
}

It is similarly recommended (and a StyleCop violation) that you put conditional clauses in brackets, so:

if (_stopped.IsDone()) return;

Should be:

if (_stopped.IsDone())
{
    return;
}

However for something as short as return; I'd be happy to let it slide.

Types

Why are you using Int32 when int works just as well?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.