Thread-safe Enum Singleton

Question

Enums are good for creating singleton. I know enum methods are not thread-safe so I tried to make it thread-safe. Can anyone please confirm if this implementation is correct or not. Is it fine to use static and volatile so many places and can it be optimized? As inner class is private so I have to create functions in enum to access inner class functionality. Can it be optimized?

import java.util.Date;

public enum SingletonWithEnum {
    INSTANCE;   

    private static class Singleton{

        private static volatile int count;
        private static volatile Date date;      

        public static  int getCount() { return count;}

        public static void setCount(int countParam) { synchronized(Singleton.class){ count = countParam; }}

        public static Date getDate() {  return date;}

        public static void setDate(Date dateParam) { synchronized(Singleton.class){ date = dateParam;}}

        public static String printObject() {
            return "Singleton [count=" + getCount() + ", date=" + getDate() + "]";
        }

    }

    public int getCount() { return Singleton.getCount();}

    public void setCount(int countParam)    {Singleton.setCount(countParam);}

    public Date getDate() { return Singleton.getDate();}

    public void setDate(Date dateParam) {Singleton.setDate(dateParam);}

    public String toString(){return Singleton.printObject();}
};

I am using it like this.

SingletonWithEnum object1 = SingletonWithEnum.INSTANCE;
object1.setCount(5);
object1.setDate(new Date());

Answer 1

First of all, you don't need a nested class in your enum. You just need to define members and methods in the enum itself, i.e.,

enum Blah {
  INSTANCE;
  private int someField;
  public int getSomeField() { return someField; }
}

Now you can access your singleton method this way:

int someField = Blah.INSTANCE.getSomeField();

Moreover, making the members static is kind of an anti-pattern here since the singleton instance should own its members. So they should be instance variables, not static variables. The fact that there is only one instance of the singleton ensures that you only have one instance of each member in your JVM.

As far as thread safety is concerned, I personally prefer atomic variables over volatile, e.g.,

private final AtomicInteger count = new AtomicInteger();
private final AtomicReference<Date> date = new AtomicReference<>(new Date());

Notice that they must be declared final in order to be truly thread safe because the atomic variables themselves will not change, although their value could.

If you only need the operations you coded, volatile variables should work. Atomic variables offer a few more operations as opposed to their volatile counterparts, e.g., compareAndSet for Java 7 and getAndUpdate and updateAndGet for Java 8. See this for a discussion.

However you declare them (atomic/volatile) if your member variables are thread safe and their thread safety policies are independent you don't need to worry about the thready safety of methods in your singleton. If you needed for example to update the two variables atomically in one shot, then you would have to rethink the design a little and introduce appropriate locks (both when setting and getting their value).

It's very important to be very careful with the way you modify your Date object. Date is not thread safe, so I highly recommend returning a copy and replacing the instance with a copy when changes are made, i.e. (assuming you are using AtomicReference as above),

public Date getDate() { return new Date(date.get().getTime()); }
public void setDate(Date d) {
  date.set(new Date(d.getTime()));
}

Finally, I highly recommend Brian Goetz's Concurrency in Practice and Joshua Bloch's Effective Java to learn more about concurrency and singleton pattern respectively.