Why this servlet code sample is thread unsafe?

Question

I saw a servlet sample code from website , this code is said to be thread unsafe,but I cannot tell why it is thread unsafe and what will happen when I use this code.This code is a servlet code which is used to save each visitor's name.

public class UnsafeGuestbookServlet extends HttpServlet {

    private Set visitorSet = new HashSet();

    protected void doGet(HttpServletRequest httpServletRequest,

             HttpServletResponse httpServletResponse) throws ServletException, IOException {

        String visitorName = httpServletRequest.getParameter("NAME");

        if (visitorName != null)

            visitorSet.add(visitorName);

    }

}

Peoper say that if I change the code like this:

private Set visitorSet = Collections.synchronizedSet(new HashSet());

thread-unsafe problem will be solved.

I know that if this piece of code is thread unsafe, it must be caused by visitorSet,it is a shared data structure.Since String visitorName is a local variable , every thread will make a copy in its own call stack, right?

Answer 1

Its simple, the UnsafeGuestbookServlet is initialized once, and the doGet or doPost methods are invoked by multiple threads parallel.

Note that the implementation of HashSet is not synchronized. If multiple threads access a hash set concurrently, and at least one of the threads modifies the set, it must be synchronized externally.

Hence to synchronize the HashSet we used Collections.synchronizedSet(). Now the HashSet is synchronized, and accesses to this particular HashSet for writing would be controlled one at a time.

If yu want more info on threading, thread safe approach why and how etc., better read this http://www.tutorialspoint.com/java/java_thread_synchronization.htm. BTW, in simple words its this, when two or more people are allowed to get a single same thing at the same time, there would be a race condition or a condition of instability. If your code permits this to happen its thread-unsafe :-)

And one more point to this, all depends on how you handle it in your code too. If you think, no 2 threads can overwrite or delete the same value from the datastructure, well you dont want to mind the thread safety. Please be aware of why thread safety is important, and in which cases its most important.

Answer 2

The HashSet in your first example is a field of the Servlet, which will be initialized only once (as the Servlet will also be initialized once). Every request will be handled by this one instance of your Servlet, therefore there is the problem with the thread-unsafeness mentioned by your example.

A HashSet is not designed to be used as a shared ressource so there may be problems like trying to add values while another thread is still iterating the Set. This is why you need some kind of synchronized Set as mentioned in your own solution.

Answer 3

Since String visitorName is a local variable , every thread will make a copy in its own call stack, right?

A copy of the pointer yes, but String class can reuse objects (as they are not modifiable) in order to save memory, so in theory you could have two threads trying to store the same object in the shared Set. And even if the objects were different ones, Set will use equals to know if the object has been already added, so your code is still exposed to unsafe check-and-add operations, iterators going through the Set while adding new elements...

Answer 4

In order to make this servlet thread-safe, I'd do two things. First, make reference to the set final:

public class UnsafeGuestbookServlet extends HttpServlet {

    private final Set visitorSet = new HashSet();    

Final references initialized at object construction time, and published safely. Without it, doGet method may not see constructed set although it has been properly initialized.

Second, either wrap set in Collections.synchronizedSet(), which is actually not the best solution because its iterators are prone for ConcurrentModificationException and all methods requires exclusive locking of the set, or use a set implementation designed for concurrent usage:

private final Set visitorSet = Collections.newSetFromMap(new ConcurrentHashMap());

This set will use striping pattern used by ConcurrentHashMap keySet which provides thread-safe iterators and way better "concurrency".

Answer 5

I know that if this piece of code is thread unsafe, it must be caused by visitorSet,it is a shared data structure.

Yes, it's a member of your servlet. The servlet container creates one instance of your servlet, but it might handle several http requests concurrently in different threads which all call your doGet() method on the same UnsafeGuestbookServlet object - which means there can be several threads manipulating the visitorSet HashSet. A HashSet is not thread safe.

Since String visitorName is a local variable , every thread will make a copy in its own call stack, >right?

Yes.