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!

possible duplicate of What is an efficient way to implement a singleton pattern in Java? — chrylis -cautiouslyoptimistic-
– chrylis -cautiouslyoptimistic-, Commented Sep 22, 2013 at 13:15
Does ConcurrentHashMap help ? It looks like that is what you need. docs.oracle.com/javase/7/docs/api/java/util/concurrent/… — Jayan
– Jayan, Commented Sep 22, 2013 at 13:18

Sergey Kalinichenko · Accepted Answer · 2013-09-22 13:18:19Z

5

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.

answered Sep 22, 2013 at 13:18

Sergey Kalinichenko

729k85 gold badges1.2k silver badges1.6k bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Mellon Over a year ago

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 Over a year ago

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

user1083009 · Accepted Answer · 2013-09-22 13:16:50Z

0

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());

answered Sep 22, 2013 at 13:16

user1083009

Comments

Srikanth · Accepted Answer · 2013-09-22 13:18:45Z

0

You can use a ConcurrentHashMap or a Collections.synchronizedMap

This article gives a good explanation

answered Sep 22, 2013 at 13:18

Srikanth

1,9561 gold badge14 silver badges20 bronze badges

Comments

Joni · Accepted Answer · 2013-09-22 13:20:21Z

0

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

edited Sep 22, 2013 at 13:20

answered Sep 22, 2013 at 13:15

Joni

112k14 gold badges151 silver badges201 bronze badges

Collectives™ on Stack Overflow

Instance variable in a Singleton class accessed by multiple threads

4 Answers 4

2 Comments

Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

2 Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related