Understanding Goetz's article on Thread safety of HttpSession

Question

Referring to Brian Goetz's article Are all stateful Web applications broken? for IBM developerWorks, I want to refer to this piece of code

HttpSession session = request.getSession(true);
ShoppingCart cart = (ShoppingCart)session.getAttribute("shoppingCart");
if (cart == null) {
    cart = new ShoppingCart(...);
    session.setAttribute("shoppingCart", cart);
}        
doSomethingWith(cart);

From my available understanding, this code is not thread-safe because it uses the check-then-act pattern. But I have a doubt:

Isn't the creation or retrieval of the HttpSession in the first line totally atomic? By atomic, I mean that if two Threads call request.getSession(), one will block. Although both will return the same HttpSession instance. Thus, if a client (mobile/web browsers) makes two or make calls to the same Servlet (that executes the snippet above), you will never get a situation in which different threads see different values for cart.

Assuming I am convinced that it is NOT thread safe, how would one make this thread safe? Would an AtomicReference work? e.g.:

HttpSession session = request.getSession(true);
AtomicReference<ShoppingCart> cartRef = 
     (<AtomicReference<ShoppingCart>)session.getAttribute("shoppingCart");
ShoppingCart cart = cartRef.get();
if (cart == null) {
    cart = new ShoppingCart(...);
    session.setAttribute("shoppingCart",
         new AtomicReference<ShoppingCart>(cart));
}
doSomethingWith(cart);

Merci!

Answer 1

Your code is still not Thread-safe:

ShoppingCart cart = cartRef.get();
if (cart == null) {
    cart = new ShoppingCart(...);
    session.setAttribute("shoppingCart",
         new AtomicReference<ShoppingCart>(cart));
}

This is because two Threads can both get a cart of null, create new shopping cart objects, and insert them into the session. One of them will "win," meaning one will set the object used by future requests, but the other will -- for this request -- use a totally different cart object.

To make this Thread-safe, you would need to do something like this, following the idiom from the article you referenced:

while (true) {
    ShoppingCart cart = cartRef.get();
    if (cart != null) {
        break;
    }
    cart = new ShoppingCart(...);
    if (cartRef.compareAndSet(null, cart))
        break;
} 

With the above code, if two Threads using the same HttpSession enter the while loop at the same time, there is no data race that can cause them to use different cart objects.

To address the part of the problem that Brian Goetz doesn't address in the article, namely how do you get the AtomicReference into the session in the first place, there's an easy and probably (but not guaranteed) thread-safe way to do this. Namely, implement a session listener and put the empty AtomicReference objects into the session in its sessionCreated method:

public class SessionInitializer implements HttpSessionListener {
  public void sessionCreated(HttpSessionEvent event){
    HttpSession session = event.getSession();
    session.setAttribute("shoppingCart", new AtomicReference<ShoppingCart>());
  }
  public void sessionDestroyed(HttpSessionEvent event){
    // No special action needed
  }
}

This method will be called once for each session, only when it is created, so this is an appropriate place to do any initialization that is needed for a session. Unfortunately, the Servlet spec does not require that there is a happens-Before relationship between calling sessionCreated() in your listener and calling your service() method. So this is apparently not guaranteed to be thread safe, and can potentially vary in behavior between different Servlet containers.

Thus, if there is even a small chance that a given session can have more than one request in flight at a time, this is not safe enough. Ultimately, in this case, you need to use a lock of some sort to initialize the session. You could do something like this:

HttpSession session = request.getSession(true);
AtomicReference<ShoppingCart> cartRef;
// Ensure that the session is initialized
synchronized (lock) {
    cartRef = (<AtomicReference<ShoppingCart>)session.getAttribute("shoppingCart");
    if (cartRef == null) {
        cartRef = new AtomicReference<ShoppingCart>();
        session.setAttribute("shoppingCart", cartRef);
    }
}

After the above code has executed, your Session is initialized. The AtomicReference is guaranteed to be in the session, and in a thread-safe manner. You can either update the shopping cart object in the same synchronized block (and dispense with the AtomicReference all together -- just put the cart itself into the session), or you can update the AtomicReference with code shown earlier above. Which is better depends on how much initialization you need to do, how long it will take to perform this initialization, on whether doing everything in the synchronized block will hurt performance too much (which is best determined with a profiler, not with a guess), and so on.

Normally, in my own code, I just use a synchronized block and don't use Goetz's AtomicReference trick. If I ever determined that synchronization was causing a liveness problem in my applications, then I would potentially move some more expensive initializations out of synchronized blocks by using tricks like the AtomicReference trick.

See also: Is HttpSession thread safe, are set/get Attribute thread safe operations?

Answer 2

Isn't the creation or retrieval of the HttpSession in the first line totally atomic? By atomic, I mean that if two Threads call request.getSession(), one will block.

Even if getSession blocks, as soon as one thread returned with the session, the lock is relinquished. While it is creating the new cart, other threads are able to acquire the lock, obtain the session, and find that there is, as yet, no cart in the session.

So, this code is not thread-safe. There is a race condition that could easily lead to multiple ShoppingCarts being created for a single session.

Unfortunately, your proposed solution is doing exactly the same thing: checking for an object in the session, and publishing one if needed, but without any locking. The fact that the session attribute is an AtomicReference doesn't matter.

To do this safely, you can use something like Goetz' "Listing 5", where the reads and writes to the session attribute are performed while synchronized on a common lock.

HttpSession session = request.getSession();
ShoppingCart cart;
synchronized (lock) {
  cart = (ShoppingCart) session.getAttribute(ATTR_CART);
  if (cart == null) {
    cart = new ShoppingCart();
    session.setAttribute(ATTR_CART, cart);
  }
}

Note that this example assumes that ShoppingCart is mutable and thread-safe.

Answer 3

So its been a few years since I've done anything with Java Servlets here so I'm going from memory.

I would expect the thread safety problem here is in the check for cart==null. When looking at threading issues the thing one must understand is that a thread can be interrupted between ANY two machine instructions (not just any line of code). That is to say, that even

i += 1;

is not threadsafe (if i is shared anyways) as i += 1 is (at least) two instructions: an add, and a store. The thread can get interrupted between the add and the store and only one of the adds will survive.

The same thing is occuring in this example. Suppose for a moment, two threads make a request on the same session (eg. as Goetz suggests from frames or ajax requests). One enters this code section, successfully retrieves the HttpSession, then attempts to get the "shoppingCart" attribute. However, since it does not yet exist, null is returned. The thread is then interrupted by another request which does the same thing. It also gets null. The two requests then proceed in any sequence, however, since both retreived a null reference for the "shoppingCart" attribute since the cart had not been stored at that time, both threads will create a new Cart object and both will try to store it. One will loose and those changes to the Cart will be lost. Thus this code is not threadsafe.

As for the second half of your question, I am not familiar with the AtomicReference object. I quickly looked over the java API for AtomicReference and it might work, but I'm not sure. In any case. The most obvious solution I can think of here is to use a monitor. Basically what you're looking to do is have mutual exclusion on the get-test-set portion of your code.

Now, provided your cart object is atomic (ie, we only need to protect the getting and setting of it, I think something like this could work:

public syncronized ShoppingCart atomicGetCart(HttpSession session){    
    ShoppingCart cart = (ShoppingCart)session.getAttribute("shoppingCart");
    if (cart == null) {
        cart = new ShoppingCart(...);
        session.setAttribute("shoppingCart", cart);
    }

    return cart;
}

HttpSession session = request.getSession(true);
ShoppingCart cart = atomicGetCart
doSomethingWith(cart);

Now, I don't know much about the performance of Java monitors so I'm not sure what kind of overhead this would incur. Also, this would have to be the only location where cart is retrieved. Basically, the syncronized keyword means that only one thread can enter the method atomicGetCart at a time. A lock is used to enforce this (a lock is simply an object that can be owned by only one thread at a time). This way you no longer have the race condition that was in the other code.

Hope this helps, -Daniel

Answer 4

Do not wanna to cross post but i wrote a comment on this article and got no answer from the author. Looking at Brian Goetz's other articles on IBM site it seems he is not keen on answering anything.

I think that code he proposed in listing 5 of his article is broken.

Let's assume that current highest score is 1000 and 2 concurrent requests with score 1100 and 1200 are on the way. Both requests retrieve the highest score in the same time:

PlayerScore hs = (PlayerScore) ctx.getAttribute("highScore");

what makes both threads see hs as being 1000. After that, one of the threads enters synchronized section, if condition is met , new value (say 1200) is set to servletcontext attribute and synchronization section ends. Then the second thread enters synchronized section and it still sees previous hs value - hs still being 1000. If contition is met (sure it is since 1100>1000) a new value (1100 ) is set to servletcontext. Shouldn't

PlayerScore hs = (PlayerScore) ctx.getAttribute("highScore");

belong to a synchronized section ?