When using a singleton pattern, should my public class return the private or public instance?

Question

I have a singleton defined like this:

public partial class MoonDataManager
{

    static MoonDataManager _singletonInstance;
    public static MoonDataManager SingletonInstance
    {
        get
        {
            return _singletonInstance;
        }
        private set
        {
            _singletonInstance = value;
        }
    }

I have a function that safely creates the instance:

   public static async Task<MoonDataManager> CreateSingletonAsync()
    {
            _singletonInstance = new MoonDataManager();

Should I:

return  _singletonInstance;  (field) 

or

return SingletonInstance;  (property)

I'm concerned with Garbage Collection, especially in iOS or Android within Xamarin.

Also if there are naming patterns for this in C# let me know if I deviated from a standard.

---

Update:

Now I think I really got myself stuck with threading and async methods. Here are the objects and their goals:

• MoonDataManager : Run the RegisterTable<Models.IssuerKey> once per table. This is a generic method that essentially runs (new MobileServiceSQLiteStore).DefineTable<T>()

• OfflineStore : This is a MobileServiceSQLiteStore.

• MobileClient : This is a MobileServiceClient.

• MoonDataManager Dependencies: The MoonDataManager requires OfflineStore and MobileClient to finish initialization. Specifically, it does a MobileServiceClient.SyncContext.InitializeAsync(OfflineStore)

I'm not sure how to make sense of this spaghetti of dependencies... or how to make the code look nice, and be thread safe.

Here is the new iteration of the code:

private readonly Lazy<MobileServiceClient> lazyMobileClient = 
        new Lazy<MobileServiceClient>(() => new MobileServiceClient(Constants.ApplicationURL), true); // true for thread safety
    public  MobileServiceClient MobileClient { get { return lazyMobileClient.Value; } }


    private readonly Lazy< MobileServiceSQLiteStore> offlineDB =
        new Lazy<MobileServiceSQLiteStore>(() =>  new MobileServiceSQLiteStore(Constants.OfflineDBName), true ); // true for thread safety
    private MobileServiceSQLiteStore OfflineStore { get { return offlineDB.Value; } }

    private static readonly Lazy<MoonDataManager> lazy =  new Lazy<MoonDataManager>(() => new MoonDataManager(), true); // true for thread safety
    public static MoonDataManager Instance { get { return lazy.Value; } }

    private MoonDataManager()
    {

             MoonDataManager.Instance.RegisterTable<Models.IssuerKey>();

            // Initialize file sync
            // todo: investigate FileSyncTriggerFactory overload. 
            //Was present on Mar 30, 2016 Channel9  https://channel9.msdn.com/events/Build/2016/P408
            MoonDataManager.Instance.MobileClient.InitializeFileSyncContext
                           (new IssuerKeyFileSyncHandler(Instance),   Instance.OfflineStore);

            // NOTE THE ASYNC METHOD HERE (won't compile)
            await MoonDataManager.Instance.MobileClient
                                 .SyncContext.InitializeAsync(MoonDataManager.Instance.OfflineStore,
                                StoreTrackingOptions.NotifyLocalAndServerOperations);

    }

Answer 1

For .NET 4 or higher, you can use the Lazy<T> and create it like this.

public sealed class Singleton
{
    private static readonly Lazy<Singleton> lazy =
        new Lazy<Singleton>(() => new Singleton(), true); // true for thread safety

    public static Singleton Instance { get { return lazy.Value; } }

    private Singleton()
    {
    }
}

It will be created only if it is accessed and only the first time and it is threadsafe.

Answer 2

The definition

static MoonDataManager _singletonInstance;

ensures that the instance of MoonDataManager is a GC root, and it will not be collected until the application domain ends, because it is a static value.

---

I'd return the private singleton and forego the auto property that you have.

public partial class MoonDataManager
{
    private static readonly Lazy<MoonDataManager> _manager = 
        new Lazy<MoonDataManager>(() => new MoonDataManager()); 

    public static MoonDataManager SingletonInstance => _manager.Value; 
}

When MoonDataManager.Value is accessed for the first time, it is initialized using the Func<MoonDataManager> that was passed to the constructor for Lazy<T>. On subsequent accesses, the same instance is returned.

Answer 3

A Singleton creates itself the first time it's accessed, in a way that ensures only one instance will get created, even if a second thread tries to access it while it's still being instantiated

your CreateSingletonAsync() violates this, and looks like it'd allow for multi-thread nastiness

You want something like:

public static MoonDataManager SingletonInstance
{
    get
    {
        if (_singletonInsatnce != null)
            return _singletonInstance;
        lock (lockobject)
        {
            // check for null again, as new one may have been created while a thread was waiting on the lock
            if (_singletonInsatnce != null)
                return _singletonInstance;
            else
                // create new one here.
        }
    }
    // no setter, because by definition no other class can instantiate the singleton
}

All this is just to ensure that two threads asking for one object don't end up creating two objects, or the second thread getting a half-created object if the first thread's one is still being created.

NB: Singletons have become unfashionable.

NB: If you can be sure that you've got time to create your object before it's ever accessed, you can just use a static member and create it on application start.

Your question "should I return the property or field" doesn't make sense -- you're already returning the field from the property getter, which is standard practise. Where else are you wanting to return something?

Answer 4

You should return the private instance. You can read more about the singleton pattern on MSDN. The standard singleton implementation is as follows:

public class Singleton
{
    private static Singleton instance;

    private Singleton() {}

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

Although, normally, you don't have a setter for the property. This pattern has already previously been discussed on SO.