RSS Feed for This PostCurrent Article

Singleton is a bad design pattern

I am not totally against Singleton design pattern. However, based on experience, normally if there is Singleton design pattern in your application, you should carefully study your code. Usually there is always a better way of doing it other than using Singleton design pattern.

Consider the following code snippet I encountered for one of the legacy Java applications.

public class MyTableAccess {
   
   private Connection myConnection;
   private MyTableAccess myInstance;
           
   public static MyTableAccess getInstance()
       throws SQLException, ClassNotFoundException {
       
       if (myInstance == null) {
           synchronized (MyTableAccess.class) {
               if (myInstance == null)
                   myInstance = new MyTableAccess();
           }
       }
       return myInstance;
   }
 
       
   private MyTableAccess() {
       setupConnection();
   }
 
   public synchronized MyRcord getRecord(String id) {
       // Retrieve the record here
   }
   
 

This piece of code was written few years back by someone who was no longer in the project. Recently this piece of code became the performance bottleneck for the entire application.

The Singleton database access class means that there will be only 1 instance shared by all threads. Since this is a multi-threaded application, the method is synchronized. When the load is high, the method becomes the bottleneck for performance. It took us quite awhile just to find out the root cause of this.

If you want to use Singleton design pattern in your application, think twice and use it if you think there is no other way out.


Trackback URL


RSS Feed for This Post5 Comment(s)

  1. Roger | May 31, 2008 | Reply

    Hello, we had a very similar case a few years ago but in the end the whole app was rewritten from scratch since there where pieces of code like this everywhere causing performance issues, however i am very interested to know how you solved this problem.

    thanks

  2. admin | Jun 1, 2008 | Reply

    Our temporary solution is to change “getInstance” to create new instance everytime the method is called. Synchronized is removed from the method. This will have minimum impact to the whole application.

    Long term, we are revamping the whole application to remove the pattern altogether.

  3. jbx | Jun 3, 2008 | Reply

    I dont agree with you that the Singleton pattern is a bad pattern. Like every weapon, it has to be used correctly.

    In your case, the bottleneck wasnt even in the getInstance() because your previous programmer made sure that the method only synchronizes when the instance is null, i.e. only the first time MyTableAccess is requested. From that point onwards, each time getInstance() is called the myInstance is not null and the synchronized block is never executed. I guess you blamed the wrong part of the code.

    In your case it is the synchronized method getRecord() that is probably causing the bottleneck not the singleton factory method. (Was the programmer afraid of some kind of concurrent access?) By creating a new instance with each getInstance() request you simply avoided the synchronisation of that method not of the singleton factory.

    In these kinds of situations, it probably makes best sense to use connection pooling (like DBCP or C3P0) with your database and use the singleton pattern with the connection pool itself. This way you can easily configure your application to allow concurrent but managed connections.

    P.S. you copied the code correctly and this line:
    private MyTableAccess myInstance;

    should have been:
    private static MyTableAccess myInstance;

    otherwise it wouldnt even have compiled.

  4. jbx | Jun 3, 2008 | Reply

    oops, sorry wanted to say:

    P.S. you copied the code ‘incorrectly’

  5. SRK | Oct 5, 2008 | Reply

    Hi,

    I have gone through the entire dicussion on the thread, I agree with JBX.

    There is no way, the singleton getinstance can cause peformance bottle neck

RSS Feed for This PostPost a Comment