[FIXED] If this is not thread safe in what ways can I make it thread safe?

Issue

Regarding HTTPSession, I am using Java Servlets and no framework. I have read about AtomicReferences but am not clear how I can use this to make this code thread safe. Am I correct in assuming I can simply mark updateSessionHashSet synchronized in providing safety? I understand this is not recommended for being inefficient. What other options and syntax is there?

Inside HTTPServlet:

private void updateSessionHashSet(HttpServletRequest req){
  HashSet<String> hs = req.getSession().getAttribute("theHashSet");
  String csv = req.getParameter("theCsv");

  String[] arr = csv.split(",");
  for(int i = 0; i < arr.length; i++){
      hs.add(arr[i].trim());
  }
}

public void doPost(HttpServletRequest  req,HttpServletResponse res) throws IOException {
   updateSessionHashSet(req);
}

Solution

The posts you're referencing are telling you that the HTTPSession is shared state and you have to be careful when multiple threads can modify it. Here the opportunity for threads to interfere is if somehow you have two concurrent POSTs from the same user that call this method. You can handle it by something like

private void updateSessionHashSet(HttpServletRequest req){
    String csv = req.getParameter("theCsv");
    Set<String> hs = new HashSet<String>();
    String[] arr = csv.split(",");
    for(int i = 0; i < arr.length; i++){
        hs.add(arr[i].trim());
    }
    req.getSession().setAttribute("theHashSet", hs);
}

This way you put together the new hashset in a local variable first, then the code overwrites the reference to the session's theHashSet attribute atomically. If two threads are calling the same code then one or the other will win, but the two won't be jumbled together (which it seems like could happen with your posted code).

Alternatively you could so like some of the answers on the linked posts advise and synchronize on the HttpSession object, but since you're synchronizing on a Java object that is local to that instance of the application, you have to look out for the case where the application is distributed across several nodes. Synchronization means using a shared lock, and across JVMs there's no sharing.

It's really common for IT departments to want to deploy an application to several instances to avoid having a single-point-of-failure. You can deal with the synchronization problem by pinning the session to a specific server, but that makes load-balancing hard.

By the way stashing stuff in the HttpSession probably isn't the best plan, you should save this in some kind of data store. The best way to have safe concurrency is to minimize the amount of mutable state. The HttpSession is not something to use as long-term storage.

For guidelines about thread-safety of Java EE stuff, I have a related answer here.

Answered By - Nathan Hughes