[MDB & singleton] pattern

[Alessandro Ilardo]

Hi there,
I'd like to have some feedback on this pattern which uses a singleton class to pass an ObjectMessage to a Message Driven Bean. What you think and how you would improve it.
The code shown belowe belongs to the client classes based on struts

public class MDBMessage {
    
    private static MDBMessage instance;
    private ServiceLocator serviceLocator;
    private QueueConnectionFactory factory;
    private Queue queue;
    private QueueConnection cnn;
    private QueueSession sess;
    private QueueSender sender;
    private ObjectMessage msg;
    
    /** Creates a new instance of MDBMessage */
    private MDBMessage() throws JMSException {
        
        serviceLocator = ServiceLocator.getInstance();
        
        factory = serviceLocator.getJMSConnectionFactory(
                Constants.MDB_QUEUE_CONN_FACT);
        
        queue = serviceLocator.getJMSDestination(
                Constants.MDB_QUEUE_MAIL);
        cnn = factory.createQueueConnection();
        
        sess = cnn.createQueueSession(false,
                QueueSession.AUTO_ACKNOWLEDGE);
        
        msg = getSess().createObjectMessage();
        
        sender = getSess().createSender(queue);
        
    }
    
    public static MDBMessage getInstance() throws JMSException {
        if(instance==null) instance = new MDBMessage();
        return instance;
    }
    
    public ObjectMessage getMsg() {
        return msg;
    }
 
    public QueueSession getSess() {
        return sess;
    }
    
    public QueueSender getSender(){
        return sender;
    }
    
}

A Struts plugin which closes the session

public class ShutDownPlugin implements PlugIn{
    
    private final Logger logger = Logger.getLogger(this.getClass().getName());
    private MDBMessage mess;
    
    public void destroy() {
        
        try {
            mess = mess.getInstance();
            if(mess.getSess()!=null) mess.getSess().close();
        } catch (JMSException ex) {
            logger.fatal("[destroy]",ex);
        }
        
        logger.debug("[destroy] removing this plugin class... ok!");
    }
    
    public void init(ActionServlet servlet, ModuleConfig config) throws ServletException {
    }
    
}

The client code which deal with the MDBMessage

MDBMessage mdb = null;
mdb = mdb.getInstance();
mdb.getMsg().setObject(email);
long sent = System.currentTimeMillis();
mdb.getMsg().setLongProperty("sent",sent);
mdb.getSender().send(mdb.getMsg());

Does it make sense?

[Jaikiran Pai]

Havent spent much time on the code, but one thing that comes to my mind is why would you make the MDBMessage singleton. Furthermore, the session and other objects like the sender are created in the constructor of this *singleton* class which means that once the ShutDownPlugin closes the session, there's no way to recreate the session and the related objects because the constructor of this singleton MDBMessage will not be called again.

[Alessandro Ilardo]

As these properties are common used by other classes, I though it was a good idea reduce the code implementing a singleton class.

The idea to close the session on the PlugIn class came out because its destroy method should be called at application server shutdown, therefore no one will be using it.

[Jaikiran Pai]

Going by this singleton approach, you are going to end up having a single connection and session sending all the messages. This means that the connection will be open throughtout the lifecycle of the application. This would not be a good idea.

As these properties are common used by other classes, I though it was a good idea reduce the code

As far as reducing the code is concerned, you can still achieve that by having this connection/session creation in a utility class which is not a singleton.

[Alessandro Ilardo]

Thanks for your reply,
but why is not a good idea? You mean memory leaks? At least I know that only one instance is using that session and therefore I perhaps minimize that risk.

[Jaikiran Pai]

With the number of posts in this forum increasing because of the book promotion, lost track of your post. Sorry about that

but why is not a good idea?

Going by the singleton approach, you are keeping one single connection open for the entire lifetime of the application. What if the connection timeouts (or breaks for some reason), you wont be able to recover since with the current design there's no way the connection will be recreated. Just my thought, let us know if you feel otherwise.