views:

88

answers:

2

I've made a class that is a cross between a singleton (fifth version) and a (dependency injectable) factory. Call this a "Mono-Factory?" It works, and looks like this:

public static class Context
{
    public static BaseLogger LogObject = null;

    public static BaseLogger Log
    {
        get
        {
            return LogFactory.instance;
        }
    }

    class LogFactory
    {
        static LogFactory() { }
        internal static readonly BaseLogger instance = LogObject ?? new BaseLogger(null, null, null);
    }
}

//USAGE EXAMPLE:
//Optional initialization, done once when the application launches...
Context.LogObject = new ConLogger();

//Example invocation used throughout the rest of code...
Context.Log.Write("hello", LogSeverity.Information);

The idea is for the mono-factory could be expanded to handle more than one item (e.g. more than a logger). But I would have liked to have made the mono-factory look like this:

public static class Context
{
    private static BaseLogger LogObject = null;

    public static BaseLogger Log
    {
        get
        {
            return LogFactory.instance;
        }
        set
        {
            LogObject = value;
        }
    }

    class LogFactory
    {
        static LogFactory() { }
        internal static readonly BaseLogger instance = LogObject ?? new BaseLogger(null, null, null);
    }
}

The above does not work, because the moment the Log property is touched (by a setter invocation) it causes the code path related to the getter to be executed...which means the internal LogFactory "instance" data is always set to the BaseLogger (setting the "LogObject" is always too late!).

So is there a decoration or other trick I can use that would cause the "get" path of the Log property to be lazy while the set path is being invoked?

A: 

A few hints:

Check out Generics

I avoid initialization using a static. This can cause odd problems in practice. For example if what you're constructing throws an error then the windows loader will tell you there's a problem but won't tell you what. Your code is never actually invoked so you don't have a chance for an exception to handle the problem. I construct the first instance when it's used the first time. Here's an example:

    private static OrderCompletion instance;

    /// <summary>
    /// Get the single instance of the object
    /// </summary>
    public static OrderCompletion Instance
    {
        get
        {
            lock (typeof(OrderCompletion))
            {
                if (instance == null)
                    instance = new OrderCompletion();
            }
            return instance;
        }
    }
Jay
This seems fairly wasteful to lock on every access, at the very least you could put a 2nd copy of the if(instance == null) outside of the lock statement to avoid locking each invocation.
Chris Marisic
I think that will create a window of opportunity to fail in a threaded environment.
Jay
+3  A: 

Note: This is a complete rewrite of the original answer; the recommendation still stands, however.

First: make sure you're not running under a debugger. For example, a watch window could touch your public static properties. This is one of the possible reasons the second example could behave differently from the first. It may sound silly, but you never know.

Under .NET 4, your second example does work, and I'd honestly expect it to work under .NET 2 as well. As long as you don't touch the Context.Log property or LogFactory.instance field inadvertently. Yet, it looks terribly fragile.

Also, strictly speaking, the beforefieldinit subtleties you're trying to use here can bite you in a multi-threaded application: the init of LogFactory does not need to run on the same thread as the setter of Context.Log[Object]. This means that when LogFactory.instance is initialized, on that thread Context.LogObject need not be set yet, while it is on another (such syncs can happen lazily). So it is not thread safe. You can try to fix this by making Context.LogObject volatile, that way the set is seen on all threads at once. But who knows what other race conditions we get into next.

And after all the tricks, you're still left with the following rather unintuitive result:

Context.Log = value1; // OK
Context.Log = value2; // IGNORED

You'd expect the second invocation of the setter to either work (Context.Log == value2) or to throw. Not to be silently ignored.

You could also go for

public static class Context
{
    private static BaseLogger LogObject;

    public static BaseLogger Log
    {
        get { return LogObject ?? LogFactory.instance; }
        set { LogObject = value; }
    }

    private class LogFactory
    {
        static LogFactory() {}
        internal static readonly BaseLogger instance 
               = new BaseLogger(null, null, null);
    }
}

Here the result is guaranteed, and lazy (in line with Jon Skeet's fifth singleton method). And it looks a lot cleaner IMHO.

Ruben
@Ruben: I've deleted my answer because I think it was misleading and/or just plain wrong. +1 for your final suggestion which makes it much more obvious what the code is actually doing.
LukeH
@Ruben: If the CLR can arbitrarily choose the init sequence, as you suggest, does that mean that Jon Skeet's singleton "fifth version" is broken (http://www.yoda.arachsys.com/csharp/singleton.html)? Anyway I'll likely modify my code, perhaps to throw an exception after the first "set", or I'll use what you proposed.
Brent Arias
No, I think Jon Skeet is right. But note there is no dependency in his code on any ordering, *and* the Nested class in his sample does not use any statics of the parent class. I think I do need to adjust my answer somewhat, because I failed to take some of the nasties of `beforefieldinit` into account.
Ruben
Yes Skeet's #5 code did not expressly show it, but gauranteed lazy initialization-order is rather the whole point of #5 - precisely so that you could modify state before or affecting the instance creation (that is why it is the only "fully-lazy" implementation). In fact, if that were not true, there would be no point in having #5 (#4 would always be sufficient).
Brent Arias
I'm not sure I agree on that one. The point of #5 is complete lazy initialization. Nothing more. But there is a possible race condition in your code (see my revised answer), that is not relevant for Jon's purpose.
Ruben
+1 for mentioning the debugger touching your properties.
Jay