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!

Read this one: http://stackoverflow.com/questions/11165852/java-singleton-and-synchronization — Konstantin Yovkov, Sep 22 '13 at 13:13
possible duplicate of [What is an efficient way to implement a singleton pattern in Java?](http://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java) — chrylis -cautiouslyoptimistic-, Sep 22 '13 at 13:15
Does ConcurrentHashMap help ? It looks like that is what you need. http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html — Jayan, Sep 22 '13 at 13:18
Why, @kocko & chrylis you think my question is duplicated with your links. It is NOT. My question is focusing on the HashMap students variable, not about how to make sure only one School instance is created in multi-thread environment. Please check my question carefully. — Mellon, Sep 22 '13 at 13:32
Hi Thanks for this question. Actually even I am trying to do something similar on the `Map` if you can provide the sample code that worked for you then it would be really helpful for me as I am pretty new to Java. Thanks in advance. — BATMAN_2008, Jul 20 '21 at 09:12

score 5 · Accepted Answer · answered Sep 22 '13 at 13:18

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.

Hi, for individual student, each thread only need to access the getter method(e.g. getStudentName()), is your 3rd point still an issue? — Mellon, Sep 22 '13 at 13:28
@Mellon If your threads only get `Student`s' properties, but not set them, then you do not need synchronization there. — Sergey Kalinichenko, Sep 22 '13 at 13:30

Joni · Answer 2 · 2013-09-22T13:20:21.663

The HashMap implementation is not thread-safe, so bad things may happen if several thread operate on it at the same time. A quick fix is making the map itself synchronized:

    students = Collections.synchronizedMap(new HashMap<String, String>());

Note that if you iterate on this map, the iteration also has to be made in a synchronized block; otherwise other threads may modify the map while you are iterating.

A thread-safe alternative to HashMap is ConcurrentHashMap

score 0 · Answer 3 · answered Sep 22 '13 at 13:16

Synchronizing method makes them thread safe it means only one thread can execute that method at a time.

However, in above situation, i would suggest to synchronized addStudent and removeStudent method only. Or you can synchronized students hash map also using -

Collections.synchronizedMap(new HashMap());

score 0 · Answer 4 · answered Sep 22 '13 at 13:18

0

You can use a ConcurrentHashMap or a Collections.synchronizedMap

This article gives a good explanation

answered Sep 22 '13 at 13:18

Srikanth Reddy Lingala

1,946
14
20

Instance variable in a Singleton class accessed by multiple threads

4 Answers4