12
\$\begingroup\$

I was using such code:

class Counter
{
    private int i = 0;

    public int Next()
    {
        lock (this)
        {
            return i++;
        }
    }

    public void Reset()
    {
        lock (this)
        {
            i = 0;
        }
    }

}

I refactored it such a way:

class Counter
{
    private int i = 0;

    public int Next()
    {
        return Interlocked.Increment(ref i);
    }

    public void Reset()
    {
        i = 0;
    }

}

Will that work?

I'm using .NET 4.5 if this matters.

\$\endgroup\$
4
  • \$\begingroup\$ I would use Thread.VolatileWrite() (or Volatile.Write()) in your Reset(), but I'm not sure whether it's actually necessary. \$\endgroup\$
    – svick
    Commented Aug 28, 2012 at 8:52
  • 1
    \$\begingroup\$ In the reset method, use System.Threading.Interlocked.Exchange(ref i, 0); \$\endgroup\$
    – user16122
    Commented Aug 28, 2012 at 9:30
  • 3
    \$\begingroup\$ Why would you make code harder to understand and possibly risk some kind of subtle issue for code that, at best, will function identically to the original code? \$\endgroup\$ Commented Aug 28, 2012 at 13:49
  • 1
    \$\begingroup\$ Whenever anyone is trying to build a lock-free construct to solve a synchronziation problem I like to point out msdn.microsoft.com/en-us/magazine/cc817398.aspx and blog.memsql.com/common-pitfalls-in-writing-lock-free-algorithms \$\endgroup\$
    – Snowbody
    Commented Jul 6, 2014 at 18:54

1 Answer 1

9
\$\begingroup\$

It should also be fine in earlier versions of .NET as you are not using any 4.5 specific classes or language features.

The reset is fine as the assignment is atomic as you mentioned, however if you were using a long then you would want to use Interlocked.Exchange because the assignment of a 64bit number is not atomic on a 32bit operating system (as it is stored as 2 32bit values).

The only changes I would suggest are purely from a coding standards perspective:

// seal the class as it's not designed to be inherited from.
public sealed class Counter
{
    // use a meaningful name, 'i' by convention should only be used in a for loop.
    private int current = 0;

    // update the method name to imply that it returns something.
    public int NextValue()
    {
        // prefix fields with 'this'
        return Interlocked.Increment(ref this.current);
    }

    public void Reset()
    {
        this.current = 0;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ are you saying that for next value you can use increment, but for resetting not used any lock stuffs. I don't it will work. Please explain better. sorry for my ignorance. \$\endgroup\$
    – sarat
    Commented Oct 27, 2012 at 11:11
  • 1
    \$\begingroup\$ You have to use Interlocked for NextValue because you need to read and write the current value as a single atomic operation, for reset you are only doing a write which is an atomic operation on its own. \$\endgroup\$ Commented Oct 27, 2012 at 11:43
  • 3
    \$\begingroup\$ I fear that the write operation in Reset, however atomic, is still not guaranteed to be seen by other threads, even those that use the Interlocked API. Since current isn't volatile the new value of 0 may never be flushed to main-memory on multi-core / multi-processor machines. See Igor Ostrovsky's elaborate explanation on the subject, and the volatile keyword. \$\endgroup\$
    – yair
    Commented Feb 12, 2018 at 12:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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