views:

178

answers:

6

I'm working on an application with a shared object that is accessed via a singleton. It's working fine on 32-bit however on 64-bit it doesn't appear to be locking properly. In the constructor for my object I have code that checks for some config reg keys and prompts the user if they don't exist. On 32 bit I see the prompt only once as expected however on 64 bit the prompt is being displayed multiple times. My code is below:

    private static readonly object padlock = new object();
    private static MyClass _instance = null;
    public static MyClass Instance
    {
        get
        {

            lock (padlock)
            {
                if (_instance == null)
                {
                    _instance = new MyClass();
                }
            }
            return _instance;
        }
    }

Any input is greatly appreciated.

Edited To Include Sample Usage:

    public OtherObject()
    {
        InitializeComponent();

        MyClass.Instance.OtherObjectOrSomething = this;

        this.Load += new System.EventHandler<EventArgs>(OtherObject_Load);
    }

Edited Again This is running inside of an Office AddIn. Thus the bitness is determined by the installation of office. I define a parameterless constructor that is private.

Thanks

Removed Slightly Anonimized Constructor

A: 

EDIT: based on some answers and comments, I'll go back to my original answer.

I've done it that way too except I would change the readonly to volatile.

private static volatile object padlock = new object();

The volatile keyword lets the compiler know not to optimize the instruction, which will ensure that the most up to date value is present in the field.

David Glass
I don't mean to be rude, so please don't take it that way, but this does not help answer the question. The OP's implementation of the singleton is acceptable; it's correct, thread-safe and simple (as long as the parameterless constructor is marked `private`). Performance suffers because a `lock` is taken every time the instance is requested, but whatever. Adding `volatile` does nothing useful here. Let's focus on understanding what the problem is.
Jason
@Jason I didn't mean to say that the volatile will fix the the issue, so I will remove that, but are you saying the the post that I linked to is incorrect about the above method failing under certain ertain multi-processor machines?
David Glass
There's one major dif to the blog tho. the lock goes around the if which is kinda the point of the article. The problem with if before lock is that multiple threads might hit the if at the same time before any other locks the syncRoot. in the above case that will not be the case
Rune FS
@David Glass: First, the question is about 32-bit machines versus 64-bit machines, not multi-processor machines. If this issue is genuine, it's very likely a CLR/JITter issue. Second, the implementation on that blog is very different than what the OP presented; that double-check locking implementation is well-known to have issues. Again, I must stress that what the OP has is a correct implementation. It's not the best, but it's correct.
Jason
I'm certainly not wed to the implementation I have. This is generally how I've done singletons for a long time however I've seem various other implementations floating around. If the use of a nested class will resolve the issue then that's an acceptable answer in my mind.
Casey Margell
@Rune-FS Thanks, I actually didn't see the diff, I immediately thought it was how I have done it and how the poster had it.
David Glass
@Casey-Margell I think Rune-FS and Jason are correct, I thought the blog post was the same as yours, it's not. It's different.
David Glass
@David Glass: But then why do you still say "[s]ee this blog post about your method not working under certain multi-processor machines"? That blog post has nothing to do with the OP's implementation.
Jason
Interestingly enough the issue seems to be resolved by switching to the fully lazy approach as outlined on the blog post linked earlier: http://www.yoda.arachsys.com/csharp/singleton.html
Casey Margell
@Casey Margell: Which makes it all the more interesting that we try to figure out what is going on. Either there is a flaw in code that you're not showing us or there is a bug in the 64-bit CLR/JITter. I'm dying to know which it is!
Jason
A: 

For an excellent discussion of the issues with the various singleton implementation patterns, see http://www.yoda.arachsys.com/csharp/singleton.html (from Jon Skeet).

As it's written, the biggest issue you have is that the compiler is providing a default parameterless constructor which will be public. You should explicitly create a paramaterless contstructor which is private to prevent this. I don't see anything that looks like it would cause an issue based on platform architecture.

Here is the code that I typically use (on both 32- and 64-bit systems):

public sealed class Singleton
{
   private static volatile Singleton instance;
   private static object syncRoot = new object();

   private Singleton()
   {
      // any code that needs to run to create a valid instance of the object.
   }

   public static Singleton Instance
   {
      get
      {
         if (instance == null)
         {
            lock(syncRoot)
            {
               if (instance == null)
               {
                  instance = new Singleton();
               }
            }
         }

         return instance;
      }
   }
}
Scott Dorman
I'm sorry, but same comment that I gave to David Glass above (http://stackoverflow.com/questions/2187090/singleton-implementation-works-fine-on-32-bit-but-not-64-bit/2187188#2187188): this does not help answer the question.
Jason
@Jason: See my updates. I suspect that the root of the problem being seen is an implementation that isn't completely correct. The architecture (32- or 64-bit) should have no bearing on how the thread locking behaves, but how the locking code in the singleton is implemented absolutely will. Implementing the private instance variable as `volatile` does have an impact on memory access.
Scott Dorman
@Scott Dorman: Yes, I agree very much that it's very likely a faulty implementation in part of the code that we are not being shown.
Jason
just a suggestion: stating explicitly that your outer if is only a performance optimazation and has nothing to do with thread-safety might be a good idea since the question can be view as a threading issue thingy
Rune FS
@Rune FS: Good point.
Scott Dorman
@Scott Dorman,you reference Jon Skeet's wonderful singleton page but then show a sample that is specifically marked `// Bad code! Do not use!` on the page. Granted, you added `volatile` so it's not as bad, but there are still several better examples on his pagethan what you quoted.
Sam
@Sam: If you read the details underneath Jon's example, this pattern doesn't end up as bad as the code comment implies. I'm not interested in this pattern working in Java as I'm not writing it in Java. If that were the case I'd use a different pattern. As for the double-checked lock, the fact that the instance variable is marked as `volatile` resolves the possible memory concerns due to the compiler semantics of the `volatile` keyword and it's easier to get right than using explicit memory barriers. Yes, it's easy to get wrong, but as long as you follow the pattern you're safe.
Scott Dorman
+2  A: 

It could be something internal to the code in the constructor causing multiple prompts. The view of the registry will be different from a 32-bit process vs. a 64-bit process so they could be responding to the differing external conditions

StarBright
A: 

I put together an implementation here, but it got downvoted twice for some reason :(

Jesse C. Slicer
A: 

Your singleton constrains you to one instance per app domain. Any chance the multiples are getting created in multiple app domains? You might look into that.

Curt Nichols
A: 

In the constructor for my object I have code that checks for some config reg keys and prompts the user if they don't exist.

This is a rather dangerous thing to do in a lazily-loaded static instance. Can you guarantee that the UI thread is always the first one to exercise this code path? I don't know for sure that this is part of your problem, but interacting with the UI inside thread-safe code is rarely a good idea, and I've seen all sorts of weird things happen in differing environments as a result.

Can you move this UI code outside the lazy loading? If this is an Office Add-In then you should have a Startup event to hook, which is guaranteed to only execute once. You don't even need the locking code that way, you can ensure that it's initialized before any other thread ever tries to touch it.

Aaronaught