Generic Singleton?

[PMarino]

Hi all - I've taken some code from MSDN and made a Generic singleton, so that
I don't have to write this code in many places:

public class Singleton<TEntity> where TEntity : class, new()

{
private static volatile TEntity instance = default(TEntity);
private static object syncRoot = new Object();

public static TEntity Instance
{
get
{
if (instance == default(TEntity))
{
lock (syncRoot)
{
if (instance == default(TEntity))
{
//instance = new TEntity();
}
}
}
return instance;
}
}
}

You would use it like this:

public class MyClass : Singleton<MyClass>
....

This works great - except that MyClass must have a public parameterless
constructor. Is there any way of getting around this? I have yet to find an
implementation that allows me to do what I want.

Thanks,Phil

[Jon Skeet [C# MVP]]

PMarino <PM*****@discussions.microsoft.com> wrote:

Hi all - I've taken some code from MSDN and made a Generic singleton, so that
I don't have to write this code in many places:

For a start, I wouldn't use that implementation. There's no point in
using double-checked locking - it's the kind of thing people try to do
to be clever, but if you're not careful it ends up not being threadsafe
:(

See http://www.pobox.com/~skeet/csharp/singleton.html

I would try to avoid having too many singletons in the first place
though - they're often (though far from always) a bad smell in design
terms. I'd keep each implementation separate.

That said, why can't you use a parameterless constructor? If you're
only going to have one of them, I can't see why you'd need any
variation between them - after all, the Instance property itself can't
specify any parameters.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[PMarino]

Hi Jon - thanks for your reply.
I'll have a look at that link.

I want to avoid making the constructors public so that client code cannot
create additional instances of the object.

Here's more detail: For my middle tier, I'm writing business objects.
Since they are stateless, they can be singletons - I'm trying to reduce the
number of objects that actually get created. There's actually no major
negative effect from creating these objects, though. And I'm making use of
generics in order to reduce code.

But I would like to keep things consistent, and the idea of a singleton is
to only have one (of course), so I'm trying to make it illegal to construct
one outside of the singleton.
"Jon Skeet [C# MVP]" wrote:

PMarino <PM*****@discussions.microsoft.com> wrote:

Hi all - I've taken some code from MSDN and made a Generic singleton, so that
I don't have to write this code in many places:

For a start, I wouldn't use that implementation. There's no point in
using double-checked locking - it's the kind of thing people try to do
to be clever, but if you're not careful it ends up not being threadsafe
:(

See http://www.pobox.com/~skeet/csharp/singleton.html

I would try to avoid having too many singletons in the first place
though - they're often (though far from always) a bad smell in design
terms. I'd keep each implementation separate.

That said, why can't you use a parameterless constructor? If you're
only going to have one of them, I can't see why you'd need any
variation between them - after all, the Instance property itself can't
specify any parameters.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Jon Skeet [C# MVP]]

PMarino <PM*****@discussions.microsoft.com> wrote:

Hi Jon - thanks for your reply.
I'll have a look at that link.

I want to avoid making the constructors public so that client code cannot
create additional instances of the object.
Ah - so it's not that you want to use a constructor with a parameter,
it's that you want to use an internal parameterless constructor. That
makes sense.

You *could* do that with reflection - you'd lose compile-time type
safety, of course, but unit tests could certainly help there.
Here's more detail: For my middle tier, I'm writing business objects.
Since they are stateless, they can be singletons - I'm trying to reduce the
number of objects that actually get created. There's actually no major
negative effect from creating these objects, though. And I'm making use of
generics in order to reduce code.

But I would like to keep things consistent, and the idea of a singleton is
to only have one (of course), so I'm trying to make it illegal to construct
one outside of the singleton.

Have you considered using inversion of control for wiring things up,
instead of embedding the knowledge of singletons all over the place?
I've been using IoC in a recent Java project with great success - it's
been *fantastically* helpful when it comes to unit tests, as I can tell
one object to use a specific interface implementation of a collaborator
for unit tests (that implementation happening to be a mock) and then in
the "real thing" using Spring to wire it up to a real implementation.

I haven't used the .NET version of Spring, but I would imagine it's
just as helpful as the Java version :)

See http://www.springframework.net

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[PMarino]

Ok, so after looking at Jon's link below, I've modified my code. The goal is
to permit the constructor to only be called ONCE, and from within the
Instance method.
There is one implementation listed in the link that would allow this very
simply - but it requires a lock() on every Instance call.

BTW: Jon, that is a VERY helpful link!

Anyway, here's the code:
public class Singleton<TEntity> where TEntity : class, new()
{
private static int numConstructorCalls = 0;

private static bool inInstance = false;
private static object syncRoot = new Object();

public Singleton()
{
int constructorCalls = Interlocked.Add(ref numConstructorCalls,
0);

//
// If the constructor has been called more than once, new() must
// have been called.
//
if (constructorCalls > 1)
{
throw new Exception("Don't call new() on Singleton");
}

//
// Check to see if this is happening from Instance
//
lock (syncRoot)
{
if (inInstance == false)
{
throw new Exception("Don't call new() on Singleton");
}
Interlocked.Increment(ref numConstructorCalls);
}
}

public static TEntity Instance
{
get
{
TEntity theEntity = null;

int isConstructed = Interlocked.Add(ref numConstructorCalls,
0);
if (isConstructed == 0)
{
lock (syncRoot)
{
inInstance = true;
theEntity = Nested.instance;
inInstance = false;
}
}
else
{
theEntity = Nested.instance;
}
return theEntity;
}
}
private static class Nested
{
// Explicit static constructor to tell C# compiler
// not to mark type as beforefieldinit
static Nested()
{
}

internal static readonly TEntity instance = new TEntity();
}
}

So the construction of the singleton is thread-safe, given the
implementation of the static variable.

Within the constructor, if the constructor has been called more than once,
and exception will be thrown. That's more of a shortcut, and doesn't need to
be there, but I thought it was a nice little performance improvement.

Then the lock is acquired, and the variable inInstance is checked. An
exception is thrown if the call is not from within the Instance method. I
don't mind doing this in the constructor, because it should only be called
once anyway.

In the Instance method, the number of constructor calls is checked. If this
is greater than one, then the object should already be constructed, and
Nested.instance is returned. If it is 0, then grab the lock and set
inInstance to true so that the first call to the constructor will not throw
an exception.

I've run multi-threaded tests and this seems to work. However, that doesn't
mean it's foolproof. I've spent a lot of time looking at this. I've
actually modified it a couple times after coming up with things that would
fail, but I can't come up with any more. Could anyone else tell me where I
might have made a mistake?
Thanks,
Phil

[Jon Skeet [C# MVP]]

PMarino <PM*****@discussions.microsoft.com> wrote:

Ok, so after looking at Jon's link below, I've modified my code. The goal is
to permit the constructor to only be called ONCE, and from within the
Instance method.
You're not calling the constructor of Singleton though. You're calling
the constructor of TEntity - and nothing is making sure that TEntity's
constructor isn't being called elsewhere as well.
There is one implementation listed in the link that would allow this very
simply - but it requires a lock() on every Instance call.
Don't be *too* worried about locking a lot - obviously it's good from a
simplicity point of view if you can avoid it, but uncontested locks are
really, really cheap.

In this case, taking out a lock each time is definitely simpler in
terms of the threading code than using interlocked in various places.
BTW: Jon, that is a VERY helpful link!
Thanks :)
Anyway, here's the code:

You seem to be ensuring it's only constructed once in three different
ways. Why not just pick one of them - preferrably the simplest?

I'd suggest just going for:

public static class Singleton<TEntity> where TEntity : class, new()
{
static TEntity instance = new TEntity();

public static TEntity Instance
{
get
{
return instance;
}
}
}

Much simpler, and just as good, I believe. It doesn't load *quite* as
lazily, but I'm not sure of a good way round that - I'm not sure
whether your nested static constructor is actually doing what you want
it to or not. Do you definitely need non-beforefieldinit behaviour?

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[PMarino]

Hi John - you're right, I'm not making sure that TEntity's constructor is not
being called. In the cases where I'm using it, though, it looks like this:

public class MyClass : Singleton<MyClass>
{
public MyClass() {}
}

I think I need to add a constraint to the Singleton class.
The problem is that I don't want someone to do this:

MyClass foo = new MyClass();

Since the Singleton requires a public constructor, this is actually allowed.
The simple implementation below would not prevent this, would it?

"Jon Skeet [C# MVP]" wrote:

PMarino <PM*****@discussions.microsoft.com> wrote:

Ok, so after looking at Jon's link below, I've modified my code. The goal is
to permit the constructor to only be called ONCE, and from within the
Instance method.

You're not calling the constructor of Singleton though. You're calling
the constructor of TEntity - and nothing is making sure that TEntity's
constructor isn't being called elsewhere as well.

There is one implementation listed in the link that would allow this very
simply - but it requires a lock() on every Instance call.

Don't be *too* worried about locking a lot - obviously it's good from a
simplicity point of view if you can avoid it, but uncontested locks are
really, really cheap.

In this case, taking out a lock each time is definitely simpler in
terms of the threading code than using interlocked in various places.

BTW: Jon, that is a VERY helpful link!

Thanks :)

Anyway, here's the code:

You seem to be ensuring it's only constructed once in three different
ways. Why not just pick one of them - preferrably the simplest?

I'd suggest just going for:

public static class Singleton<TEntity> where TEntity : class, new()
{
static TEntity instance = new TEntity();

public static TEntity Instance
{
get
{
return instance;
}
}
}

Much simpler, and just as good, I believe. It doesn't load *quite* as
lazily, but I'm not sure of a good way round that - I'm not sure
whether your nested static constructor is actually doing what you want
it to or not. Do you definitely need non-beforefieldinit behaviour?

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Jon Skeet [C# MVP]]

PMarino <PM*****@discussions.microsoft.com> wrote:

Hi John - you're right, I'm not making sure that TEntity's constructor is not
being called. In the cases where I'm using it, though, it looks like this:

public class MyClass : Singleton<MyClass>
{
public MyClass() {}
}

I think I need to add a constraint to the Singleton class.
The problem is that I don't want someone to do this:

MyClass foo = new MyClass();

Since the Singleton requires a public constructor, this is actually allowed.
The simple implementation below would not prevent this, would it?

No. No constraint you can put on the singleton would prevent the type
from being constructed "manually". You can only do that by making the
type itself a singleton, rather than providing it *via* a singleton.

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too