Windowscommunitytoolkit: The Singleton<T> class should be removed

[Sergio0694]

This issue relates to the Singleton<T> type defined in Microsoft.Toolkit.Helpers.Singleton.cs.

I'm wondering whether having this type in the toolkit at all is a good idea, considering it can be fully replaced by only using built-in C# language features, which also result in a faster execution.

The current implementation of the Singleton<T> class is as follows:

public static class Singleton<T>
    where T : new()
{
    // Use ConcurrentDictionary for thread safety.
    private static ConcurrentDictionary<Type, T> _instances = new ConcurrentDictionary<Type, T>();

    /// <summary>
    /// Gets the instance of the Singleton class.
    /// </summary>
    public static T Instance
    {
        get
        {
            // Safely creates the first instance or retrieves the existing instance across threads.
            return _instances.GetOrAdd(typeof(T), (t) => new T());
        }
    }
}

This works, but there are a few issues with this implementation:

• That _instances field is within a generic type! This means that for each Singleton<T> type, that field will be different, and an entirely new ConcurrentDictionary<Type, T> instance will be built. Furthermore, since each T instance is only created once, it means that that dictionary will never really be used at all. When the Instance property is accessed, _instances will always either be empty, or with a single object of type T, since that's the one being looked for in the current Singleton<T> type. Might as well just use a statically initialized field.
• The thread-safety here is guaranteed by that _instance being statically initialized, since the static class constructor is run under a lock (see here and here). But this means that the same thread-safe behavior could be achieved without the need of a Singleton<T> class in the first place.

Consider this snippet:

public class MyClass
{
    public static MyClass Instance { get; } = new MyClass();
}

This is translated to (see here):

public class MyClass
{
    [CompilerGenerated]
    private static readonly MyClass <Instance>k__BackingField = new MyClass();

    public static MyClass Instance
    {
        [CompilerGenerated]
        get
        {
            return <Instance>k__BackingField;
        }
    }
}

Which again, causes the C# compiler to emit this code in the static constructor for MyClass:

.method private hidebysig specialname rtspecialname static 
    void .cctor () cil managed 
{
    newobj instance void MyClass::.ctor()
    stsfld class MyClass MyClass::'<Instance>k__BackingField'
    ret
}

And this static constructor (see linked issues above) is run under a lock in a given app-domain and only executed once, This is all that's necessary to ensure that a given T instance is only created once when the static T Instance property is accessed the first time (since doing so triggers the static constructor).

Suggestions:

• Remove the Singleton<T> class entirely from the toolkit.
• Provide docs to inform developers of this built-in technique to implement a T Instance field for the singleton pattern, without the need of additional APIs at all.

[msftbot[bot]]

Hello Sergio0694, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will investigate the issue and help solve it ASAP. Other community members may also look into the issue and provide feedback 🙌

[yanxiaodi]

@Sergio0694 I think it would be better to add a private constructor for MyClass to prevent other classes from instantiating it because that would violate the singleton pattern.
I agree with your opinion.

[Sergio0694]

@yanxiaodi Thanks! 😄
I didn't add it there just because it wasn't strictly in the scope of the issue (this was just about the Singleton<T> class being unnecessary), and because in some cases uses would still want to leave the singleton class available for other instances (eg. the Messenger class in the MVVMLight library comes to mind, as it exposes a handy Messenger.Default singleton property while still giving developers the ability to spawn other messenger instances if needed, or to inherit from it).

But yeah in general I completely agree, in most situations one should also add a private constructor to make sure that developers don't use the class improperly 👍

[Sergio0694]

This was fixed in https://github.com/windows-toolkit/WindowsCommunityToolkit/pull/3435, closing this 🚀