Synchronized and locks in singleton factory

Question

I have a singleton factory (edit: renamed "loader" to avoid confusion with factory pattern) that creates objects (in my example DAOs) or returns them if already created:

public class DAOLoader {

    private static final DAOLoader INSTANCE = new DAOLoader();

    private UserDAO userDAO;
    private MessageDAO messageDAO;

    private final Object lockUserDAO = new Object();
    private final Object lockMessageDAO = new Object();

    private DAOLoader() {}

    public static DAOLoader getInstance() {
        return INSTANCE;
    }

    public UserDAO getUserDAO() {
        if (userDAO == null) {
            synchronized(lockUserDAO) {
                if (userDAO == null) userDAO = new UserDAO();
            }
        }
        return userDAO;
    }

    public MessageDAO getMessageDAO() {
        if (messageDAO == null) {
            synchronized(lockMessageDAO) {
                if (messageDAO == null) messageDAO = new MessageDAO();
            }
        }
        return messageDAO;
    }
}

First, do you guys see anything wrong with this code?
In this example, is a different lock for each method required or should I just use 1 global lock? Would a deadlock happen with a unique global lock? If not, the only drawback would be that if the lock is used by some thread to create a DAO and another thread would like to create another DAO, it would have to wait for the lock to be released?

Thanks.

Chew

Only drawback? Lots more than this. Globally visible objects like won't scale. Google has a Singleton hunter to identify and expunge them: reddit.com/comments/ikf9z – Tinhorn

I don't think there's even a factory design in there. check this tutorialspoint.com/design_pattern/factory_pattern.htm – Fadden

@Tinhorn "Globally visible objects like won't scale" what do you mean? – Chew

@NathanHughes this is a project where I can not use Spring or any DI framework, I just want to create DAOs 1 time and reuse them. – Chew

Let me make it easy for you: Don't create a Singleton or all this locking code. Pool your connections and use them in the narrowest scope possible. Learn Spring - they wrote a better JDBC framework than you ever will. – Tinhorn

@Ramanlfc singleton eager loading is on purpose; why the getXXX() method don't belong here and what is wrong with the locking? Please explain, this is why I ask ;-) – Chew

@Tinhorn I use spring-jdbc, what's wrong with having unique reusable DAOs? Isn't it how it would be if you were using a DI framework? – Chew

@NathanHughes why do the variables need to be volatile? DAOs are created only 1 time and never modified so when a thread gets one it won't change. On the other hand I think I am going to remove static. – Chew

because Java Memory Model: once one thread creates a new DAO, when a second thread checks the value outside of the sync block, there's no guarantee the value written by the first thread will be seen by the second thread. – Higgins

@NathanHughes hmm, if thread #2 doesn't see it it will try to create it right? Then it enters the synchronized block and tests again that the DAO is null before creating it. Are you saying that different threads may get different values for members of a singleton? – Chew

Nobody who uses Spring does all that locking stuff. You should set the isolation level of the connection appropriately using @Transaction annotations and a transaction manager. – Tinhorn

You need volatile but not because of that. Without volatile, memory operations order is not guaranteed, so your dao reference can be non-null but still only be partially initialized (can have empty uninitialized fields). Btw most of these guys have no idea why they are dissing singleton, there is nothing wrong with it – Ernaldus

@Tinhorn what I mean is if you use Spring DI, you will most likely inject a DAO (or repository) in your service or resource/controller class, this DAO will be created by the DI framework as a singleton (application scoped). I am trying to reproduce the same behavior, creating "reusable beans". – Chew

Why "reproduce the behavior"? Spring isn't making a synchronization mess like this. – Tinhorn

@Tinhorn I haven't looked under the hood of Spring framework ;-) Since you have, maybe you can recommend a better design to manage singleton DAOs? Oh and I guess Spring may not need synchronization because it creates all beans at startup, not on demand. – Chew

@Ernaldus I am confused, after exiting synchronized blocks, objects created in such blocks may not be completely initialized? – Chew

@MaximeLaval no, the problem is that the outermost null check is not going to respect anything happening inside the synchronized block so it can happen at a point when the dao is partially initialized inside the synchronized block and see it is not null and return it in an inconsistent memory state – Ernaldus

@Ernaldus hmm ok, I thought the "state" of an object would change from null to non null when it is completely initialized... – Chew

I already have: Use Spring as written and learn about the transaction manager. Get rid of that Singleton mess. – Tinhorn

@Tinhorn I guess there is a misunderstanding, I said I can not use Spring DI (IoC container) for that specific project, I have to manage the lifecycle of my objects myself. How would you lazy load safely singletons on demand since you think my code is wrong? Thanks! – Chew

Re, Is a different lock for each method required or... Locks aren't for methods, locks are for data. Use locking to prevent other threads from seeing/acting on/modifying data structures when one thread must put the data into a temporary, invalid state in order to do its work. You can use different locks when you have different data structures that do not have to be mutually consistent with one another. – Orourke

"cannot"? You should. You will never, ever write anything that will be as good as what Rod Johnson has given you. Mutlithreaded code is hard to write well. Even smart people get it wrong. I would not use singletons; I would not lazy load any such thing because I KNOW that I'm going to use it. Such constructs are usually put in place by developers who are smart enough to have heard of them but not smart enough to realize that they're extra, unnecessary complication. – Tinhorn

@MaximeLaval: If you still want to use SIngleton, have a look at this article to fix double locking issue: javarevisited.blogspot.in/2014/05/…. I prefer to avoid lazy initialization with static block or use Enum. – Nazario

also consider singletons that manage their own scope like this are hard to test because you can't just plug in a mock implementation. that's one of the reasons for making DI frameworks like Spring in the first place. I disagree with @duffymo's wording above, nothing about this is about smarts, it is more about experience. I lived through the pre-spring period of roll-your-own-frameworks and I'm kind of done with it. – Higgins

@ravindra yeah I saw that article but I eagerly initialize my singleton so static is fine. I guess I'll add volatile to my DAO members. – Chew

@NathanHughes I agree about using a DI framework and testing issues with singletons, but right now for that project I have some technical constraints that prevent me from using a IoC container... – Chew

Answer 1

Your example seems a bit confused because you're preventing the DaoLoader's constructor from being visible but you're not preventing the Dao constructors from being visible. Also having a loader class can turn into a dumping ground for things, and it encourages organizing by layer rather than by feature.

You might consider using the Initialization-on-Demand holder idiom:

public class UserDao {
    private UserDao() {}

    String findById(Long id) {
        return "foo";
    }

    private static class LazyUserDaoHolder {
        static final UserDao USER_DAO_INSTANCE = new UserDao();
    }

    public static UserDao getInstance() {
        return LazyUserDaoHolder.USER_DAO_INSTANCE;
    }
}

The holder static class isn't initialized until the method accessing it is called, since it is initialized on first access (and class initialization is serial) no synchronization is required.

Higgins

@Ravindra: that's the getInstance method here. i took the liberty of simplifying the posted example and getting rid of the DaoLoader. – Higgins

I think it is better than double locked singleton. BTW, is USER_DAO_INSTANCE need to be volatile? – Nazario

@Ravindra: no, it doesn't need to be volatile. Because it is final, it's safely published when the class is initialized. – Higgins

Got it. I have lost in double locking blues of lazy singleton earlier :) – Nazario

@NathanHughes As I said in the comments under my question, eager initialization is on purpose. The DAO constructors are visible because they may or may not be used through the loader, also I pass parameters to them (like a datasource). Anyway, my question was more about unique/same locks and synchronization ;-) – Chew