Let's say we have this code in a servlet container environment, java/j2ee. It is a basic web application. We have monitoring tools that mention that this class is getting majority of locks during execution of the request thread. Is that only because of the singleton approach here? If there are locks, could this code run into race conditions? Cause a high volume of execution time? The locks are on the singleton class.
public class SomeSingleton {
private static final Thing object = new Thing();
public static SomeSingleton instance = null;
private final Properties logfiles = new Properties();
public static SomeSingleton getInstance() {
if (instance == null) {
createInstance();
}
return instance;
}
/**
* Imagine this method called
* SomeSingleton.getInstance().log()
*/
public void log(final String message) {
try {
synchronized (logfiles) {
}
}
}
Servlet:
@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
SomeSingleton.getInstance().log()
}
enum.ReentrantLock in long-running (blocking) code. Use synchronized only around brief code that does not block, such as checking guards in memory.Getting Java locks on a singleton class, understanding why
Not "locks" plural, but singular. There is only a single lock in your code, from this line: "synchronized (logfiles) {".
The locks are on the singleton class.
The lock is not on your SomeSingleton class, if that is what your meant by "the singleton class". The lock is on the Properties object held in logfiles object.
SomeSingleton
Your class is not actually a singleton, as explained in Answer by Eggen.
You have a race condition, where multiple threads could be accessing your getInstance method and read a null before you have finished assigning an instance to SomeSingleton instance. At runtime, your code could actually instantiate more than one SomeSingleton objects.
Plus, SomeSingleton instance should be private rather than public if you intend access to go through getInstance.
You need to revamp that code. I would change it around to make a single instance available through a public final object reference, and make the constructor private to assure no inadvertent instantiations take place.
Change this:
public class SomeSingleton {
private static final Thing object = new Thing();
public static SomeSingleton instance = null;
private final Properties logfiles = new Properties();
public static SomeSingleton getInstance() {
if (instance == null) {
createInstance();
}
return instance;
}
/**
* Imagine this method called
* SomeSingleton.getInstance().log()
*/
public void log(final String message) {
try {
synchronized (logfiles) {
}
}
}
… to the following.
We mark instance as public because this is our designated access route for calling methods.
We mark instance as final to prevent re-assignment to another object.
We re-org our static vars together for clarity.
We add a constructor, making it private to avoid uncontrolled instantiation.
We initialize logfiles in the constructor, rather than on the declaration line, as a matter of style. Some folks like me want to see all the initialization for this object in one place, not scattered amongst declaration lines. Reasonable people may disagree.
This code instantiates instance in the static declaration, … instance = new SomeSingleton();. We no longer need your getInstance method. This instantiation occurs when the class loads. The instance field is marked final to prevent any other instance from being assigned.
public class SomeSingleton
{
// Statics.
public static final SomeSingleton instance = new SomeSingleton();
private static final Thing thing = new Thing();
// Member fields.
private final Properties logfiles; // To enable logging.
// Private constructor.
private SomeSingleton()
{
this.logfiles = new Properties() ;
}
/**
* Perform logging by calling:
* SomeSingleton.instance.log( … )
*/
public void log( final String message )
{
synchronized ( this.logfiles )
{
…
}
}
}
ReentrantLock rather than synchronized for virtual threadsFor compatibility with virtual threads in Java 21+, we replace synchronized with a ReentrantLock object.
Do this only if (a) the work being performed is not entirely CPU-bound (involves blocking), and (b) takes a while to execute. If short-lived, just use synchronized. Pinning the virtual thread is not a problem for a mere moment.
public class SomeSingleton
{
// Statics.
public static final SomeSingleton instance = new SomeSingleton();
private static final Thing thing = new Thing();
// Member fields.
private final Properties logfiles; // To enable logging.
private final Lock loggingLock; // To protect logging.
// Private constructor.
private SomeSingleton()
{
this.logfiles = new Properties() ;
this.loggingLock = new ReentrantLock();
}
/**
* Perform logging by calling:
* SomeSingleton.instance.log( … )
*/
public void log( final String message )
{
this.loggingLock.lock(); // Blocks until lock becomes available.
try
{
… // Involves blocking, such as I/O.
}
finally
{
this.loggingLock.unlock();
}
}
}
enumBut even this is not ideal. Singleton pattern is surprisingly tricky business!
Current wisdom is that a singleton in Java is generally best implemented as an enum. For more info, see:
An enum is implicitly static and final, no need to add those modifiers.
In transforming to an enum, we can delete the line: public static final SomeSingleton instance = new SomeSingleton();. The named enum object populates (the instantiation of our singleton) when the class loads, just the same as our static field did as mentioned above.
public enum SomeSingleton
{
// enum
INSTANCE ;
// Statics.
private static final Thing thing = new Thing();
// Member fields.
private final Properties logfiles; // To enable logging.
private final Lock loggingLock; // To protect logging.
// Private constructor.
private SomeSingleton()
{
this.logfiles = new Properties() ;
this.loggingLock = new ReentrantLock();
}
/**
* Perform logging by calling:
* SomeSingleton.INSTANCE.log( … )
*/
public void log( final String message )
{
this.loggingLock.lock(); // Blocks until lock becomes available.
try
{
… // Involves blocking, such as I/O.
}
finally
{
this.loggingLock.unlock();
}
}
}