Instance variable in a Singleton class accessed by multiple threads

Question

I have a singleton class:

public class School {
    private HashMap<String, String> students;

    private static School school;

    private School(){
        students = new HashMap<String, String>();   
    }

    public static School getInstance(){
       if(school == null){
           school = new School();
       }
       return school;
    }

    //Method to add student
    protected void addStudent(String id, String name){
          students.put(id,name);
    }
    //Method to remove student
    protected void removeStudent(String id){
          students.remove(id);
    }
}

As you can see above, in the singleton class, I have a students variable (a HashMap), there are methods to add & remove student in the class.

In my application, there could be multiple threads using this School class to getInstance() , then adding & removing student. To make the access (especially the access to students instance) be thread safe, I am thinking to use synchorized keyword for getInstanc() method, like:

public synchronized static School getInstance(){
       if(school == null){
           school = new School();
       }
       return school;
    }

But I think my trivial change only can make sure only one School instance be created in multi-thread environment. What else do I need to do in order to make it thread safe for accessing the students instance by multiple threads as well. Any good suggestion or comment is appreicated, thanks!

Answer 1

Leaving the conversation on whether singletons are evil or not, let's consider only the thread safety issues in your School class:

• The shared object is created "lazily" - this needs synchronization to avoid making two instances of School; you have correctly identified and fixed this issue. However, since initializing School does not take much time, you might as well make getInstance() a trivial getter by initializing school = new School() eagerly.
• The hash map inside the School - concurrent access to the hash map will result in exceptions. You need to add synchronization around the code that adds, removes, and iterates students to avoid these exceptions.
• Access to individual students - once the callers get a Student object, they may start modifying it concurrently. Therefore the Student object needs concurrency protection of their own.