Synchronizing the HttpSession

This is something that I have heard a great deal of debate over the last 2 years about. The servlet spec was somewhat recently amended to clarify that there is no guarantee that multiple calls to HttpServletRequest.getSession() or HttpServletRequest.getSession(boolean) will return the same object. This holds especially true in containers that return a facade object that wraps around the actual HttpSession object that you are working with, like Tomcat.

Why would you want to synchronize a session anyway?
The answer is pretty simple actually. Consider the following theoretical block of code:

public class WithdrawFundsServlet extends HttpServlet {

 @Override
 protected void doPost(HttpServletRequest request, HttpServletResponse response)
   throws ServletException, IOException {
  User u = ESAPI.authenticator().getCurrentUser();
  String withdrawAmt = request.getParameter("withdrawAmt");
  float amt;
                Account acct = session.getAttribute("acct_"+u.getAccount());
  try
  {
   amt = Float.parseFloat(withdrawAmt);
  }
  catch ( Throwable t )
  {
   ESAPI.log().info( Logger.SECURITY_FAILURE, "Non-Numeric value passed as Withdraw Amount");
   try {
    ESAPI.httpUtilities().sendForward(request, response, "/error" );
   } catch (AccessControlException ignored) { }
  }
  
  // Calling Withdraw will queue a check to be printed and mailed to the customer.
  AccountFacade.withdraw( acct, amt );
  
  try
  {
   ESAPI.httpUtilities().sendForward(request, response, "/success" );
  }
  catch (AccessControlException ignored) { }
  
  return;
 }
}

Now there are a couple things that I will point out that I am sure you will notice if you are paying attention. The first is that yes, this example is using the ESAPI. Call it a shameless plug :). The second is that I am ignoring AccessControlExceptions. This is purely to keep this example scenario short and to the point, and in any production code, you would never want to do this. There would also be some validation code in there as well.

Aside from those things, it looks innocent enough right? Well let's consider this for a second with a scenario.

Joe needs to have a check cut to him from his account at SomeBodiesBank. So he gets online and hits the form for the above servlet. Joe is not that savvy of a computer user, and like most novice internet users will do, he has the tendency to double-click on everything. He fills out the form to withdraw $500 from his account and double-clicks the submit button. So somewhere on the backend, we'll say in the AccountFacade.withdraw method, the software validates that Joe has enough money to cover the check, it discovers he has $750 in his Checking account so everything looks good. But wait a minute, Joe double-clicked remember?

Do you know what happens when you double click the submit button on a form? Well, 2 requests get submitted one after the other. Hmmmmmm.. So now I have 2 requests entering this method at the exact same time, both requests check Joe's balance and discover that he has $750 in his account, so they both queue up a request to print a check for the requested amount. There's only one problem, these are cashiers checks, the bank has withdrawn $1000 dollars (or in some circumstances, maybe only withdrew the original $500 from his account) but Joe ended up with $1000 in cashiers checks!

The checks show up in the mail, and Joe being the responsible individual he is, reports this to the bank. The bank will likely write this off as an anomoly and the bug will remain until one day when Joe is down on his luck and remembers the bug. He finds a program called JMeter and submits 1000 requests to the servlet as fast as he can for $1000 withdrawals. When his 1,000,000 in cashiers checks arrive, he promptly leaves the country and disappears in the backwoods of New Zealand never to be heard from again.

So the moral of the story is that this problem could have been easily avoided simply by adding thread-safety measures to the code. Granted the example cited is extreme and the consequence of the example even more extreme, but I can promise you that something similar to the situation has already happened and even moreso I can guarantee that something similar will happen again.

So, with this knowledge, what is the correct means to add thread safety around manipulating the session. It's quite simple even.

final Object lock = request.getSession().getId().intern();
synchronized(lock) {
   AccountFacade.withdraw( acct, amt );
}

Would do the trick in this simple example.

It's important when using synchronization to always lock on immutable objects. It is also important to use the same lock when locking in multiple places where you are working with the same data. Thread-safety is an entire subject on it's own that is will beyond the scope of this blog posting, so I will cut to the chase here.

This is incorrect, and not guaranteed:

synchronized (request.getSession()) {
   // do stuff
}

While this method is proven and works:

synchronized (request.getSession().getId().intern()) {
   // do stuff
}

Some interesting stats to close out with:

Google Code Search Results found approximately 4000 uses of different ways to say 'synchronized(session)'

The scary part is this was only the first 5 ways I came up with to search for it.