11

This is the first time I am writing a Factory class. Below is my Factory class, I am not sure whether this is the correct way of making thread safe Singleton Factory class or not. I will be returning instance of my Client using this factory?

public class ClientFactory {

    private static ClientFactory instance = null;   

    private ClientFactory() {

    }

    public static ClientFactory getInstance() {

        if (instance == null)
        {
            instance =  new ClientFactory();
        }

        return instance;
    }

    public IClient getClient() {

        return new TestClient();
    }
}

And here is my TestClient class - 

public class TestClient implements IClient {


}

And this is how I am going to use my factory - 

IClient client = ClientFactory.getInstance().getClient();
  • You have only one instance of your factory that return a new instance of Client everytime you call getClient.. what's the problem? :) – gipinani Jan 09 '14 at 23:15
  • Is this the correct way of making a factory pattern that will return only one instance? I was reading about some IDIOM that I can use for this as well.. –  Jan 09 '14 at 23:16
  • @SSH Ignore my previous answer. Your factory is a singleton. If that's what you're after, you're there. And your usage is OK. – peter.petrov Jan 09 '14 at 23:17
  • Aah.. That's what I am looking for.. I will update my question accordingly.. How to make thread safe singleton factory? And also why it is not thread safe, if you can explain me that as well, then it will be of great help.. –  Jan 09 '14 at 23:21
  • Read also this: http://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java/71399#71399 – MariuszS Jan 10 '14 at 08:13

5 Answers5

24

In fact your factory isn't thread safe, because in race condition you can have more than one ClientFactory living in application. Lets assume two threads:

  1. ThreadA is evaluating condition 'if (instance == null)' and instance is null, so it enters statement
  2. ThreadB is evaluating condition 'if (instance == null)' and instance is null (because A didn't make to instantiate it), so it enters statement
  3. ThreadA creates new ClientFactory() and returns it
  4. ThreadB creates new ClientFactory() and returns it
  5. Now we have more than one ClientFactory in application. Of course other threads trying to retrieve instance some time later will always return single instance.

In my opinion the easiest way to write singleton in Java is to use enum. In your case it will looks:

public enum ClientFactory {
  INSTANCE;

  public Company getClient() {
    return new Company();
  }
}

And usage:

ClientFactory.INSTANCE.getClient()
Jakub Kubrynski
  • 13,724
  • 6
  • 60
  • 85
5

Singletons and Factories are different things. To property construct a Singleton, I guess you could think of its getInstance() method as a factory. Factories make "things". Singleton means that there will only ever be 0 or exactly 1 of these "things" in existence at any time.

If you are trying to make a proper Singleton, it is surprisingly cumbersome to do this in a Thread-safe manner in Java. Without synchronization or other thread-safe countermeasures, the code you list above has a subtle race-condition around the check-then-set code to initialize ClientFactory instance variable. There are two ways around this race-condition. Which way you pick is largely gated by how expensive it is to go through the ClientFactory constructor. My constructors are typically lightweight, so I go the path of avoiding the need for synchronization all together.

public class ClientFactory {
    private static final ClientFactory instance = new ClientFactory();

    private ClientFactory() { }

    public static ClientFactory getInstance() {
        return instance;
    }
}

If you want to be "lazy" in the construction, not building on until someone explicitly calls getInstance(), now synchronization is needed to avoid the race condition.

public class ClientFactory {
    private static ClientFactory instance = null;

    private ClientFactory() { }

    public static synchronized ClientFactory getInstance() {
        if ( instance == null ) {
            instance = new ClientFactory();
        }
        return instance;
    }
}
Bob Kuhar
  • 10,838
  • 11
  • 62
  • 115
  • Thank you.. Can you explain little bit in detail why my code is not thread safe singleton with an example? I am just trying to understand.. –  Jan 09 '14 at 23:37
  • Consider the case where 2 different threads at almost exactly the same moment call getInstance(). Both of them see instance == null and both of them new ClientFactory() assigning it to instance. They both succeed in and everything seems to "work". But rather than just a single ClientFactory() having been created, there have been two; one of which is no longer reachable because no-one holds a reference to it. Your code is not thread-safe because it lacks synchronization protection around the check-then-set. – Bob Kuhar Jan 09 '14 at 23:49
  • 1
    The static final variant avoids the synchronization issue entirely and generally works pretty well, but failures in static constructors are a drag to track down (StackTraces with no method name are just wonderful...not). – Bob Kuhar Jan 10 '14 at 01:14
5

Thread safe implementations(examples) on Wiki - Singleton Pattern on Wikipedia

As in the link above, a single-element enum type is the best way to implement a Singleton for any Java that supports enums.

One of the best yet simple ones:

public class ClientFactory{
    private ClientFactory() {}

    private static ClientFactory INSTANCE=null;

    public static ClientFactory getInstance() {
        if(INSTANCE==null){
            synchronize(ClientFactory.class){
                if(INSTANCE==null) // check again within synchronized block to guard for race condition
                    INSTANCE=new ClientFactory();
            }
        }
        return INSTANCE;
    }
}

Source: Wikipedia

ram
  • 747
  • 2
  • 11
  • 34
2

Your factory is a perfect Singleton (it is just that it is not thread-safe).

peter.petrov
  • 38,363
  • 16
  • 94
  • 159
  • Thanks for suggestion.. Can you explain little bit in detail why it is not thread safe singleton? I am just trying to understand.. –  Jan 09 '14 at 23:33
  • @SSH Assume your instance is currently null. Assume two threads call the static getInstance() method at about the same time. If Thread 2 calls this line "if (instance == null)" right after Thread 1 called this same line, but before Thread 1 has called the next line, then both Threads will see that the instance is null, and both will go ahead and create instances of your factory (so you will have 2 instances now) thus breaking your Singleton pattern. – peter.petrov Jan 09 '14 at 23:50
  • If you define "singleton" as "only one instance per process," then the OP's code is _not_ a perfect singleton, in that it can result in multiple instances per process. – yshavit Jan 10 '14 at 00:27
0

the ClientFactory is a factory,but neither a Singleton Factory,nor a thread-safe factory. At any point,when ClientFactory.getInstance().getClinet() is invoked,it will return a new instance,so it is not absolutely a Singleton Factory.If we fix the method like this

private IClient iclient;

public IClient getClient() {

    if ( iclient == null ){
         iclient = new TestClient();
    }

    return iclient ;
}

Then the factory is a singleton factory,but it is not thread-safe. Assume if there are more than one threads invoke getInstance,all of the threads will find that the client factory instance is null,so they will construct the instance respectively, and the problem is the same with the method getClient().

It is very easy to fix it,you can declare these two method as synchronized.

First of all,if you really want to use factory parrtern,don't forget to hidden the client's Constructor

private TestClient(){
}
sambean
  • 1