CODEWITHSUNDEEP
javajpasingleton
BrendanMcQ
BrendanMcQ

Reputation: 31

EnityManagerFactory Singleton. Please just check out?

I am just cruious if this looks solid. It gives no errors but I just want to double check as I am having a pooling issue with c3p0. Just checking to see if anything here is the cause. Thank you in advance!

import javax.persistence.EntityManagerFactory;
import javax.persistence.Persistence;


public class EntityManagerFactorySingleton {
    private static EntityManagerFactorySingleton singleton;
    private EntityManagerFactory emf;

public EntityManagerFactorySingleton(){
    emf = Persistence.createEntityManagerFactory(ConfigList.getProperty(Config.PERSISTENCE_UNIT), System.getProperties());
}

public synchronized static EntityManagerFactorySingleton getInstance() {
   if(singleton == null) {
    singleton = new EntityManagerFactorySingleton();
   }
   return singleton;
}

public EntityManagerFactory getEntityManagerFactory(){
    return emf;
}

}

Upvotes: 0

Views: 919

Answers (3)

Aaron Digulla
Aaron Digulla

Reputation: 328760

  1. The code is not thread safe. (Sorry, missed the synchronized)

  2. You should not use singletons.

Instead, use a DI framework like Spring, guice or maybe your deployment envionment already offers one.

This will make your code much more robust and much more simple to test.

Upvotes: 0

Costi Ciudatu
Costi Ciudatu

Reputation: 38255

Your code is not "solid":

  • constructor must be private for a singleton
  • you shouldn't have the getInstance() method synchronised, although you need to perform the initialisation thread-safe. That's because after initialization, all the threads that need the instance will have to wait for each other (and that's a useless bottleneck). Only if your instance is null, call a synchronized (private) method that performs the initialisation; inside that method, check again if the instance is null. Another approach is to have a private inner class SingletonHolder that holds the instance, so you'll rely on the class-loader for performing the thread-safe initialisation.

However, if you can't (don't want to) avoid using a singleton, a very good choice would be an enum with only one constant defined: INSTANCE;

public enum EntityManagerFactorySingleton {
    INSTANCE;

    // all your code -- fields, constructor, instance / static methods get in here
    // you can still have the `getInstance()` static method returning INSTANCE, if you want.
}

The only drawback is that you cannot perform lazy initialisation for the INSTANCE, but you're now thread-safe and ready for serialization or cloning issues without any effort.

Upvotes: 2

planetjones
planetjones

Reputation: 12633

To answer your question - I think you should make the constructor private, to ensure other Classes cannot instantiate another EntityManagerFactorySingleton. However, if that is not happening, then I can see no reason why this class would be causing a pooling issue.

Upvotes: 0

Related Questions