Problem while creating Singleton

Question

I am trying to create a Singleton class, which will be accessed from two other classes.Can anyone please tell me whats wrong with the following code? I am just not able to figure out!

import java.util.LinkedList;

public class MessageQueue {

    private static final LinkedList<ServerDataEvent> queue = new LinkedList<ServerDataEvent>();;

    private static MessageQueue messageQueue = null;

    /** A private Constructor prevents any other class from instantiating. */
    private MessageQueue() {
    }

    /** Static 'instance' method */
    public static MessageQueue getInstance() {
        if (MessageQueue.messageQueue == null) {
            System.out.println("Creating MessageQueue instance.");
            MessageQueue.messageQueue = new MessageQueue();
        }
        return MessageQueue.messageQueue;
    }

    public Object clone() throws CloneNotSupportedException {
        throw new CloneNotSupportedException();
    }
}

I am accessing the singleton object from other classes like this:

MessageQueue messageQueue = MessageQueue.getInstance();

There are no errors, but

System.out.println("Creating MessageQueue instance.");

is getting executed whenever I do

MessageQueue messageQueue = MessageQueue.getInstance();

EDIT 1

import java.util.LinkedList;

public class MessageQueue {

    private static final LinkedList<ServerDataEvent> queue = new LinkedList<ServerDataEvent>();;

    private static final MessageQueue messageQueue = new MessageQueue();

    /** A private Constructor prevents any other class from instantiating. */
    private MessageQueue() {
        System.out.println("problem...");
    }

    /** Static 'instance' method */
    public static MessageQueue getInstance() {
        return MessageQueue.messageQueue;
    }

    public Object clone() throws CloneNotSupportedException {
        throw new CloneNotSupportedException();
    }
}

What's wrong here is your first sentence: I am trying to create a Single ton class, which will be instantiated from two other classes. - you see the problem? It seems like a singleton is not what you want. — Björn Pollex
– Björn Pollex, Commented Jun 28, 2011 at 17:46
is there an error that occurs? is the singleton not created? — Hunter McMillen
– Hunter McMillen, Commented Jun 28, 2011 at 17:47
This is not thread-safe. Also, what is the problem referred to in the question title? — nicholas.hauschild
– nicholas.hauschild, Commented Jun 28, 2011 at 17:48
The logic for singleton seems to be correct. What problem do you see? Are you getting compile time errors or is the logic not working for you? — dev_musings
– dev_musings, Commented Jun 28, 2011 at 17:48
The basics of the code look fine, other than the fact it is not thread safe. What problems are you experiencing? Not getting an instance? Other? — Gregory A Beamer
– Gregory A Beamer, Commented Jun 28, 2011 at 17:49

Amir Rachum · Accepted Answer · 2011-06-28 17:45:30Z

4

First of all, you did not specify any errors you get. If you want to get help, you should give us as much information as you can.

Secondly, the best fool-proof way to create a singleton in Java is this:

public enum MySingleton {
    INSTANCE;

    //whatever methods you want to implement
}

and you access it like so: MySingleton.INSTANCE.whatever().

answered Jun 28, 2011 at 17:45

Amir Rachum

80.3k74 gold badges174 silver badges253 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Sachin Over a year ago

There are no errors, but System.out.println("Creating MessageQueue instance."); is getting executed whenever I do MessageQueue messageQueue = MessageQueue.getInstance();

anubhava · Accepted Answer · 2011-06-28 17:45:00Z

3

It is much better to define and instantiate your singleton object like this:

private static final MessageQueue messageQueue = new MessageQueue();

And then getInstance will be just:

public static MessageQueue getInstance() {
   return MessageQueue.messageQueue;
}

This way your singleton object is instantiated and will be thread safe because it is created by the class loader.

answered Jun 28, 2011 at 17:45

anubhava

790k67 gold badges603 silver badges671 bronze badges

5 Comments

anubhava Over a year ago

@Michael J. Lee: Thanks a lot, realized the typo and fixed :)

Sachin Over a year ago

@anubhava: I did what you said. But I also added a simple System.out.println("inside constructor"); line in the constructor. After this if I do MessageQueue messageQueue = MessageQueue.getInstance(); from 2 different classes, I can see inside constructor both the times. Is it expected? Thanks.

RHSeeger Over a year ago

Be aware that having the instance created by the thread loader can cause problems if creating the instance can throw an Exception. If it does, then the class will fail to be loaded and the error message will be... less than useful to people using your class (ie, you just get a message that it couldn't load any such class).

anubhava Over a year ago

@Bhushan: Did you change your getInstance() method as well as I suggested above? As you can see construction is not even happening in getInstance() anymore so no matter how many times you call getInstance() it should never print inside constructor.

anubhava Over a year ago

@RHSeeger: That's a valid point. Besides that another situation where instance created by the loader can't be used is when constructor needs some run time parameters to be passed.

Peter Lawrey · Accepted Answer · 2011-06-28 17:50:28Z

1

A shorter version which is thread safe.

public enum MessageQueue {
    INSTANCE;

    private final Queue<ServerDataEvent> queue = 
        new ConcurrentLinkedQueue<ServerDataEvent>();    

    public void addEvent(ServerDataEvent event) { queue.add(event); }
}

or

public enum MessageQueue {
    ;

    private static final Queue<ServerDataEvent> queue = 
        new ConcurrentLinkedQueue<ServerDataEvent>();    

    public static void addEvent(ServerDataEvent event) { queue.add(event); }
}

answered Jun 28, 2011 at 17:50

Peter Lawrey

535k83 gold badges770 silver badges1.2k bronze badges

Comments

Michael J. Lee · Accepted Answer · 2011-06-28 17:47:49Z

0

would be easier if you did it this way...

public class MessageQueue {

    private static final MessageQueue INSTANCE= new MessageQueue();

    public static MessageQueue getINSTANCE() {
        return INSTANCE;
    }

    private MessageQueue() {
    }
}

answered Jun 28, 2011 at 17:47

Michael J. Lee

12.5k3 gold badges25 silver badges39 bronze badges

Comments

Mike Caputo · Accepted Answer · 2011-06-28 17:50:15Z

0

What's the error that occurs? All I can see from this is you have two semicolons here:

private static final LinkedList<ServerDataEvent> queue = new LinkedList<ServerDataEvent>();;

answered Jun 28, 2011 at 17:50

Mike Caputo

1,15617 silver badges33 bronze badges

Comments

Sachin · Accepted Answer · 2011-08-31 17:36:17Z

0

Simplest method:

private static final MessageQueue messageQueue = new MessageQueue();

public static MessageQueue getInstance() {
   return MessageQueue.messageQueue;
}

answered Aug 31, 2011 at 17:36

Sachin

18.9k33 gold badges110 silver badges141 bronze badges

Collectives™ on Stack Overflow

Problem while creating Singleton

6 Answers 6

1 Comment

5 Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

1 Comment

5 Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related