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?