Singleton Pattern with Public Constructor

Question

public class MySingletonClass
{
  public MySingletonClass()
  {
    _mySingletonObj = Instance();
  }

  public static MySingletonClass Instance()
  {
    if (_mySingletonObj  == null)
    {
      lock (typeof(lockObject))
      {
        if (_mySingletonObj  == null)
          _mySingletonObj  = new MySingletonClass();
      }
    }
    return _mySingletonObj ;
  }
}

MySingletonClass _myObj = new MySingletonClass();

This act as singleton with public constructors..?

Thanks

I think this code results in a dead lock and a recursive constructor call -> stack overflow. — Oliver Hanappi
– Oliver Hanappi, Commented Sep 16, 2009 at 9:07
This code is full of problems, and does not in fact work as a singleton, due to the public constructor, which as Oliver has noted, will also result in a stack overflow. — Lasse V. Karlsen
– Lasse V. Karlsen, Commented Sep 16, 2009 at 9:15

Jon Skeet · Accepted Answer · 2009-09-16 09:13:52Z

16

No, it's not a singleton - anyone can create multiple instances of it. (Leaving aside the stack overflow issue that's already been raised, and the fact that you're using double-checked locking unsafely.)

One of the distinguishing features of a singleton type is that it prevents more than one instance of itself from ever being constructed.

From the wikipedia Singleton Pattern article:

In software engineering, the singleton pattern is a design pattern that is used to restrict instantiation of a class to one object.

From Ward Cunningham's pattern repository:

A Singleton is the combination of two essential properties:

Ensure a class only has one instance

Provide a global point of access to it

Clearly your singleton fails to meet both these definitions.

See my singleton article for real implementations.

edited Sep 16, 2009 at 9:13

answered Sep 16, 2009 at 9:03

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Lasse V. Karlsen · Accepted Answer · 2009-09-16 11:59:11Z

4

The code as posted does not work as a singleton, due to the public constructor, but in addition to that, there's numerous flaws and problems with the code:

Public constructor calls Instance, which calls public constructor, which calls Instance, which calls..... stack overflow imminent
Public constructor returns a new object every time it is called, you can not replace the returned result with the same object reference upon subsequent requests. In other words, public constructor on singleton breaks the pattern.
You should not leak your lock objects, which in your case you lock on the type of an object. Do not do that, instead lock on a private object.

Here's a fixed version of your code:

public class MySingletonClass
{
  private readonly object _mySingletonLock = new object();
  private volatile MySingletonClass _mySingletonObj;

  private MySingletonClass()
  {
    // normal initialization, do not call Instance()
  }

  public static MySingletonClass Instance()
  {
    if (_mySingletonObj == null)
    {
      lock (_mySingletonLock)
      {
        if (_mySingletonObj  == null)
          _mySingletonObj = new MySingletonClass();
      }
    }
    return _mySingletonObj;
  }
}

MySingletonClass _myObj = MySingletonClass.Instance();

edited Sep 16, 2009 at 11:59

answered Sep 16, 2009 at 9:18

Lasse V. Karlsen

394k107 gold badges651 silver badges845 bronze badges

1 Comment

Jon Skeet Over a year ago

That's still not thread-safe - the _mySingletonObj would need to be volatile.

SwDevMan81 · Accepted Answer · 2009-09-16 11:43:20Z

3

Check out the .NET Optimized code section at the Dofactory. This has IMO the best implementation. Also check out the site for other design pattern implementations in C#.

answered Sep 16, 2009 at 11:43

SwDevMan81

50.2k24 gold badges155 silver badges191 bronze badges

Comments

Adriaan Stander · Accepted Answer · 2009-09-16 09:04:30Z

1

The constructor should be private

answered Sep 16, 2009 at 9:04

Adriaan Stander

167k32 gold badges294 silver badges288 bronze badges

Comments

Heiko Hatzfeld · Accepted Answer · 2009-09-16 09:19:57Z

1

Instead of locking it, you could also try to create a static readonly singelton...

public sealed class Singleton
{
    private static readonly Singleton instance = new Singleton();

    static Singleton()
    {
    }

    private Singleton()
    {
    }

    /// <summary>
    /// The public Instance property to use
    /// </summary>
    public static Singleton Instance
    {
        get { return instance; }
    }
}

answered Sep 16, 2009 at 9:19

Heiko Hatzfeld

3,19520 silver badges15 bronze badges

Comments

ZuTa · Accepted Answer · 2012-07-01 10:40:13Z

1

The essence of Singleton is to provide :

exactly one instance of class across the system;
simple access to it.

The implementation of Singleton based on creation a class with a method(or property in .NET) that creates an instance of this class if one doesn't exists yet. The constructor of class must be private to prevent other ways of initialization. Also Singleton must be carefully used in multi-threaded applications because in one point of time, the two threads may create two different instances (which violates singleton pattern).

More details and examples you can find here.

answered Jul 1, 2012 at 10:40

ZuTa

1,4641 gold badge11 silver badges19 bronze badges

Collectives™ on Stack Overflow

Singleton Pattern with Public Constructor

6 Answers 6

Comments

1 Comment

Comments

Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

Comments

1 Comment

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related