Hoan Nguyen
Hoan Nguyen

Reputation: 18151

Is this method of create Singleton thread safe?

This is not a discussion of whether or not Singleton is good or bad. It is about creating singleton. The way I understand Singleton is that it is a class that at anytime there should exists at most of one object. That is if several classes instantiate a singleton at the same time then they would share a single instance of that singleton.
The problem with singleton is that once it is created it will exist for the duration of an application. With my method you can create and have a Singleton garbage collected any time if it is not being use any more. Well I need to have an enum (Singleton!) to create all other Singleton. I think my method is reflection and serialized safe but I am not sure if there is any problem with thread.
My method to create singleton is as follow:

First any class that want to be singleton has to extend the following class

public abstract class Singleton {
    public Singleton(SingletonFactory.SingletonParam singletonParam) {
        if (singletonParam == null) {
            throw new NullPointerException("singletonParam cannot be null)");
        }
    }

    // For singleton to release resources.    
    public abstract void destroy(SingletonFactory.SingletonParam singletonParam);
}

SingletonParam is going to be an abstract class inner class which cannot have an object which is a SingletonParam in a polymorphism sense instantiate outside of its container class.
This is not intended to have subclasses extends the Singleton class at runtime. It is for classes that are singletons by having a static instance. My method does not require any static instance of the singleton class.
The container is the following class

Note: after reading the answer from Stephen C, I made the change to initialize the HashMap from the constructor and I do not see why it is not thread safe.

public enum SingletonFactory {
    INSTANCE;

    enum SingletonList {
       A,
       B,
      ......
    }

   private final HashMap<String, SingletonInfo> mfSingletonInfoHashMap = new HashMap<>();

    // Added after @Stephen C answer
    SingletonFactory() {
        mfSingletonInfoHasmap.put(A, final new SingletonInfo());
     // put all the members of the SingletonList here.   
        At this time the  Singleton member of the SingletonInfo is null.   
        It will be instantiated when a class call getSingleton
     }

    private class SingletonInfo {
        final Set callingObjects = new HashSet();
        Singleton singleton;
    }

   public Object getSingleton(SingletonList inList, Object object) {
      final SingletonInfo singletonInfo = mfSingletonInfoHashMap.get(inList);
       synchronized (singletonInfo) {
              if (singletonInfo.callingObjects.add(object)) {
                  if (singletonInfo.singleton == null) {
                      singletonInfo.singleton = createSingleton(singletonClassName);
                    }
              } else {
                  throw new RuntimeException("getSingleton(" + singletonClassName + ") has already been called and not released");                 
              }
      return singletonInfo.singleton;
  }

    public void releaseSingleton(SingletonList inList, Object object) {
          SingletonInfo singletonInfo = mfSingletonInfoHashMap.get(inList);

          synchronized (singletonInfo) {
            singletonInfo.callingObjects.remove(object);
            if (singletonInfo.callingObjects.isEmpty()) {               
                singletonInfo.singleton.destroy(new SingletonParam() {
            });
            singletonInfo.singleton = null;
          }
      }
  }

    private Singleton createSingleton(SingletonList inList) {
        switch(inList) {
           case SingletonA:
               return new SingletonA(new SingletonParam() {});
           ......
        }
    }

    public abstract class SingletonParam {
        private SingletonParam() {

        }
    }
}

Well the above is not quite right since you have to extends the HashSet that contains the CallingObjects in the SingletonInfo to implement equality by reference and not equal in the default implementation.

Since an instance of a subclass of SingletonParam cannot be instantiate outside of SingletonFactory the only way you can create a singleton object is by calling SingletonFactory.INSTANCE.getSingleton(you_singleton_class_name, this)

Upvotes: 3

Views: 274

Answers (1)

Stephen C
Stephen C

Reputation: 719576

The original version is not thread-safe. The getSingleton method was reading and updating the HashMap without synchronizing on anything.


I see there maybe a problem when there is no SingletonInfo associates with a singleton class name yet.

Correct.


This is about the updated version of the code:

In that case I can just populate my hashmap with all the singleton class names with empty CallingObjecs and null Singleton and then synchronize on the SingletonInfo. Does that sound right?

Yes, provided that:

  1. the initialization happens in the SingletonFactory constructor,
  2. the hashmap is safely published, and
  3. nothing changes the hashmap after it has been the factory has been constructed.

However, that would seem to defeat one of the advantages of your initial implementation; i.e. that the singleton factory is no a priori knowledge of the singletons.

A better approach (i.e. an approach that preserves the advantages of your original attempted solution) would be to either synchronize access to the HashMap, or use a ConcurrentHashMap ... and its special atomic operations.


Since I learn programming just by coding and no formal training ...

You at least need to read a good text book on Java Concurrency, if you are going to learn to use Java concurrency properly. Try Goetz et al.

(You need need to understand the principles of Java concurrency. You are unlikely to learn them by trial and error.)

It is (IMO) concerning that you are so focused on making the singleton design pattern work, when the "received wisdom" is that singletons are a bad idea. Have you considered Dependency Injection and its advantages?

The other thing is that these are not true singletons.

  • The implied semantics of your Singleton class contradict its name. There can be multiple instances of Singleton ...

  • There is nothing to prevent someone from creating multiple instances of a given Singleton subclass. Given your implementation approach, there needs to be a non-private constructor for each suclass in order for the factory to work.

  • Unless you declare your Singleton subclasses as final, that would provide another route to breaking singleton-ness.

In short, I'm not convinced that this infrastructure you are building is going to achieve your goal of implementing (true) singletons better.

Upvotes: 4

Related Questions