Singleton class is not working

Question

I'm trying to make Clients class singleton, but it is not working. Here is my class:

public class Clients {
    private static Clients instance = null;
    private ArrayList<Client> cList;

    private Clients() {
        cList = new ArrayList<Client>();
    }

    public static Clients getInstance() {
        if (instance == null) {
            System .out.println("created");
            instance = new Clients();
        }

        return instance;
    }

    public static ArrayList<Client> getcList() {
        return getInstance().cList;
    }

    public static void setcList(ArrayList<Client> cList) {
        getInstance().cList = cList;
    }
}

I am getting this instance in two different classes(both have their own main function). After getting its instance in one class, I get it in another class, but both tiare still executing.

Answer 1

Whenever implementing a singleton, the getInstance() method should be thread-safe.

e.g.,

public static synchronized Clients getInstance()

... or ...

private static final Object INSTANCE_LOCK = new Object();

public static Clients getInstance() {
    synchronized(INSTANCE_LOCK) {
        if(instance == null) instance = new Clients();
    }
    return instance;
}

Of course, if you're in fact executing this bit of code from two different programs rather than two different threads, you'll have two instances. I'm assuming the former, because the latter makes your question nonsensical.

I suppose I should explain why that's nonsensical.

When you execute a Java program with a main(String[] args) method, all of your classes are loaded into the JVM. If you then execute another program, you get another JVM and another "copy" of all the associated classes. Thus, you have two separate singletons -- one for each program. Classes aren't shared between the two.

Answer 2

You mentioned that both classes "have their own main", so I am assuming you have two separate programs.

Long story short, data isn't really shared between two programs. A singleton class will ensure you only have one instance of that object within a single program, but two programs will still be completely independent of each other and cannot share data this way.

This would be the case even if you only had one class with a "main" and just ran it twice.

If you want to share data between programs like this, you have many, many options, but some are:

• See if you can actually combine your two separate programs into one. Do you really need two programs?
• Use a database to store your data, MySQL and SQLite are two easy options, among many.
• One program can write data to a file, and the other program can read it.
• There are many other options to send data from one program to another, such as sockets (there are a zillion network protocols that already exist, plus you could roll your own), platform-specific things like named pipes on Windows, shared memory, etc. Check out the Google results for "java ipc" (Inter-Process Communication -- these are general techniques for allowing two programs to communicate with eachother).

Answer 3

You could use a synchronized block above as Jeff Gohlke has stated, but you may also want to look into using locks.

The best thing about locks are that synchronized keyword doesn’t provide fairness whereas we can set fairness to true while creating ReentrantLock object so that longest waiting thread gets the lock first.

// Fairness set to false is faster than a synchronized block.
private static final ReentrantLock rlock = new ReentrantLock(false);

public static final Clients getInstance() {
    rlock.lock();
    try {
        System.out.printf("[Thread %s] Clients.getInstance()%n",
            Thread.currentThread().getName());
        if (instance == null) {
            instance = new Clients();
        }

        return instance;
    } finally {
        rlock.unlock();
    }    
}

Answer 4

You can get rid of a lot of your problems by just doing

private static Clients instance = new Clients().

No need to worry about the locking idiom in get instance. There's not point in doing this sort of lazy instantiation unless the class that you are building is expensive to build.

Having said that, I'm not sure I like the getter and setter for the cList either. I'd prefer these to be instance methods on the class you are making a singleton, so you'd do ( for example )

Clients clients = Clients.getInstance();
clients.getcList();

And, if this is a multithreaded environment then you'd have to be aware that the setter may affect other threads that already have a refernce to the singleton object.