11
\$\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\$
  • \$\begingroup\$ I would use Thread.VolatileWrite() (or Volatile.Write()) in your Reset(), but I'm not sure whether it's actually necessary. \$\endgroup\$ – svick Aug 28 '12 at 8:52
  • 1
    \$\begingroup\$ In the reset method, use System.Threading.Interlocked.Exchange(ref i, 0); \$\endgroup\$ – user16122 Aug 28 '12 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\$ – David Schwartz Aug 28 '12 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 Jul 6 '14 at 18:54
7
\$\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\$
  • \$\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 Oct 27 '12 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\$ – Trevor Pilley Oct 27 '12 at 11:43
  • 1
    \$\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 Feb 12 '18 at 12:52

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.