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!

Solution

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.

Comments

[Mellon]

Hi, for individual student, each thread only need to access the getter method(e.g. getStudentName()), is your 3rd point still an issue?

[Sergey Kalinichenko]

@Mellon If your threads only get Students' properties, but not set them, then you do not need synchronization there.