Is my service thread safe?

Question

I have a singleton class MyService, it has two functions to read & write data from/to file:

public class MyService {
 private static MyService instance;
 private MyFileAccessor fileAccessor;

 private MyService() {
   fileAccessor = new MyFileAccessor();
 }

 public static MyService getInstance() {
  if (instance == null) {
     instance = new MyService();
  }
  return instance;
 }
 // write data to file through fileAccessor object
 public void writeDataToFile(Object data){
   fileAccessor.writeToFile(data);
 }
 // read data from file through fileAccessor object
 public Object readFile() {
   return fileAccessor.readFromFile();
 }

}

MyFileAccessor class has synchronized functions to read & write file:

public class MyFileAccessor {
  private File mFile;

  public MyFileAccessor() {
    mFile = new File(PATH);
  }
  public synchronized Object readFromFile() {
    // code to read mFile
    …
  } 

  public synchronized void writeToFile(Object data) {
    // code to write data to mFile
    …
  } 
}

My question : Is the singleton MyService class thread-safe when my project reading & writing data from/to file through it? Are there potential concurrency issues?

===== UPDATE ===

Two more questions based on the answers:

Q1. I see the answers below about using Initialized-On-Demand idom. But isn't it enough if I just use the synchronized keyword on getInstance() static method?

public static synchronized MyService getInstance() {
 ...
}

Doesn't it also make the singleton instance creation atomic?

Q2. If I only use MyFileAccessor through MyService instance, is it still necessary to make MyFileAccessor a singleton or synchronized on MyFileAccessor.class ? I mean MyService is a singleton, isn't it already guaranteed only one instance is able to invoke methods in MyFileAccessor?

Answer 1

You don't actually have a singleton class at the moment. Because you check if instance == null and then maybe assign instance = new MyService() non-atomically, there is a chance that two threads will create instances of MyService.

One way to create a thread-safe singleton is to use a single-element enum:

enum MyService {
  INSTANCE;

  // Rest of class body.
}

You can now get your instance via MyService.INSTANCE.

An alternative is the Initialization-on-demand idiom, which makes use of the fact that a class is not initialized until it is first required:

class MyService {
  private static class Holder {
    private static final MyService INSTANCE = new MyService();
  }

  static MyService getInstance() {
    return Holder.INSTANCE;
  }
}

As @Kayaman notes below, the singleton enum pattern is the currently-preferred way of doing it. I can think of reasons why you might want to use the IOD idiom, for instance if you need to extend another class (enums can't extend classes, since they already extend Enum; however, they can implement interfaces).

For completeness, yet another pattern is Double-checked Locking, which uses a synchronized block if instance is found to be null:

static MyService getInstance() {
  if (instance == null) {
    synchronized (MyService.class) {
      if (instance == null) {
        instance = new MyService();
      }
    }
  }
  return instance;
}

---

Having done that, you should also make all fields final: there is a happens-before guarantee on the assignment of final fields with respect to the constructor finishing execution.

private final MyFileAccessor fileAccessor; // In MyService.

private final File mFile;  // In MyFileAccessor.

Otherwise, there is no guarantee that these fields' values will be visible to all threads.

---

You should also make MyFileAccessor either a singleton (e.g. using the lazy holder idiom), or make the methods synchronized on MyFileAccessor.class, to ensure that only one instance is able to invoke those methods at the same time.