8

I saw in a mprss book this recommendation of singleton (partial code attached):

public static Singleton GetSingleton() 
{
    if (s_value != null) 
        return s_value;
    Monitor.Enter(s_lock); 
    if (s_value == null) 
    {
        Singleton temp = new Singleton();
        Interlocked.Exchange(ref s_value, temp);
    }
    Monitor.Exit(s_lock);
    return s_value;
}

We add two lines of code in the 2nd if statement block instead of just writing:

s_value = new Singleton();

this should handle a situation that a second thread enters the method and finds s_value != null, but not initialized.

My question is, can we just write at the 2nd if block instead:

{    
    Singleton temp = new Singleton();
    s_value = temp;  // instead of Interlocked.Exchange(ref s_value, temp);    
}

So now the function is:

public static Singleton GetSingleton() 
{      
    if (s_value != null)
        return s_value;

    Monitor.Enter(s_lock);   
    if (s_value == null)   
    { 
        Singleton temp = new Singleton();    
        s_value = temp; 
    }   
    Monitor.Exit(s_lock);   
    return s_value; 
}

I guess not, because they don't use it.

Does anyone have any suggestions?

It is possible that svalue may contains uninitialized? svalue can be constructed just after temp was fully initialized (may i wrong). if i wrong can u point of an example it is wrong? may the compiler produce differenent code?

roman
  • 607
  • 2
  • 10
  • 18

2 Answers2

8

I'll post this not as a real answer but as an aside: if you're using .NET 4 you really should consider the Lazy<T> singleton pattern:

http://geekswithblogs.net/BlackRabbitCoder/archive/2010/05/19/c-system.lazylttgt-and-the-singleton-design-pattern.aspx

public class LazySingleton3
{
   // static holder for instance, need to use lambda to construct since constructor private
   private static readonly Lazy<LazySingleton3> _instance
       = new Lazy<LazySingleton3>(() => new LazySingleton3());

   // private to prevent direct instantiation.
   private LazySingleton3()
   {
   }

   // accessor for instance
   public static LazySingleton3 Instance
   {
       get
       {
           return _instance.Value;
       }
   }

}

Thread-safe, easy to read, and obvious: what's not to like?

Jeremy McGee
  • 24,842
  • 10
  • 63
  • 95
  • 2
    I'm not sure if the question asked for the perfect (lazy) singleton pattern, or wanted to understand why the assignment is done with the help of the `Interlocked` class. – Lucero Dec 29 '11 at 13:14
  • @Lucero: yes, but my Java and Scala-loving friends tend to laugh at the way that C# implementations of singleton flail around with details that really should be hidden. The more I can evangelize `Lazy` the better. ;-) – Jeremy McGee Dec 29 '11 at 13:17
  • the major question is why the line of the interlocked can't be change with "s_value = temp". – roman Dec 29 '11 at 13:34
  • It is possible that svalue may contains uninitialized? svalue can be constructed just after temp was fully initialized (may i wrong). if i wrong can u point of an example it is wrong? may the compiler produce differenent code? can you please give a detaild example? – roman Jan 01 '12 at 15:23
4

While the assignment is atomic, it is also required that the memory is "synchronized" across the different cores/CPUs (full fence), or another core concurrently reading the value might get an outdated value (outside of the synchronization block). The Interlocked class does this for all its operations.

http://www.albahari.com/threading/part4.aspx

Edit: Wikipedia has some useful information about the issues with the double-locking pattern and where/when to use memory barriers for it to work.

Lucero
  • 59,176
  • 9
  • 122
  • 152
  • +1 Good answer. Also see [this one for more detail](http://stackoverflow.com/questions/2192124/reference-assignment-is-atomic-so-why-is-interlocked-exchangeref-object-object). – user703016 Dec 29 '11 at 13:13
  • Assume that each core has its own memory cache. Now if core A writes the reference into `s_value` and concurrently core B reads it (without a memory fence inbetween) core B will not read the freshly assigned value but instead get the old (null) value. – Lucero Dec 29 '11 at 13:18
  • so core b should get null at the first if and wait for the lock. i don't understand what is the problem with this – roman Dec 29 '11 at 13:54
  • 3
    But the lock code also causes a memory barrier, so why is the interlocked operation necessary? – Eric Lippert Dec 29 '11 at 16:00
  • @EricLippert, I don't consider myself an expert on it any you may want to chat with your coworker [Joe Duffy](http://www.bluebytesoftware.com/blog/SearchView.aspx?q=timeliness) for details about it - I'd love to hear the expert's answer to it. My understanding is that the execution order is not guaranteed to be sequential depending on the memory model (read and write reordering), which requires this additional memory barrier. Java had this problem and they "fixed" all those buggy double-check implementations by changing the memory model. – Lucero Dec 29 '11 at 17:11
  • 1
    @Lucero: Indeed; double-checked locking gives me a bad feeling every time I think about it. I don't have any confidence whatsoever that any implementation of it is correct. – Eric Lippert Dec 29 '11 at 17:15
  • i want to mention again this code was taken from the book "CLR via C#, 3rd edition" – roman Dec 29 '11 at 17:21
  • @tomera, if you read [Joe's articles on the topic](http://www.bluebytesoftware.com/blog/SearchView.aspx?q=%22double%20checked%22) the book seems to be correct. Note that the ECMA specs make weaker guarantees than the MS CLR, so that some code may be "correct" on the MS stack and/or some of the runtimes, but not on others (Mono comes to my mind). – Lucero Dec 29 '11 at 17:28
  • but he show an example that using boolean field like:temp = new Singleton(); initialized = true;============= and at my example: as i understand: and still may i wrong: s_value = temp; can be execution only after full construction of : Singleton temp = new Singleton(); – roman Jan 01 '12 at 15:20
  • @tomera, whether it is a boolean or a reference doesn't matter, since both can be written in an atomic operation. If you look at the examples given with the boolean you'll notice that they as well need the additional memory barrier even though the boolean is set inside of the `lock` region. The same therefore applies to changing references. – Lucero Jan 01 '12 at 16:58
  • @Lucero: at joe example: he is showing why it's can leak because he his checking the condition against the boolean field. but at this example i check against the null field. and for my opinion this is different result. can i chat with you? – roman Jan 03 '12 at 11:02
  • @tomera, sorry, I'm only sporadically online so chatting will be difficult. – Lucero Jan 03 '12 at 23:40