Don’t Let Your Non Thread-Safe Variables Escape!

I was asked to help debug a problem in a project that was about to be deployed. It was one of those ‘fun’ transient problems that only happens when you can least afford it. Like when the customer is testing the product.

The project consists of a JSP/AJAX web tier that connects to the DB via a JPA EntityManager. The problem we had is that every now and then the user would see a nasty assertion error for an uncaught exception displayed in their browser. (Why the exception was not handled nicely is a another subject worth writing about).

The code uses a custom static class to provide access to the EnityManager (Using a DI framework instead would be nice). It has some actions that span multiple calls so the EntityManager for each Thread was stored in a ThreadLocal variable. This should ensure that each Thread should get its’ own EntityManager and hence its’ own transaction. I noticed a few synchronization problems when I first looked at the class. For example:

public static EntityManager getEntityManager() {
  EntityManager entityManager = 
     managerThreadLocal.get();
  if (entityManager == null 
                          || !entityManager.isOpen()) {
    if (entityManagerFactory == null) {
      entityManagerFactory = 
        Persistence.createEntityManagerFactory("myDS");
    }
    entityManager = 
      entityManagerFactory.createEntityManager(); 
    managerThreadLocal.set(entityManager);
  }
  return entityManager;
}

So to fix this we made the entityManagerFactory a private static final member variable of the class. Since the enityManager itself is ThreadLocal we shouldn’t need to synchronize access to that variable since only one Thread should be ever accessing its’ own EntityManager at any one time.

Of course this did not fix our main problem. The real problem turned out to be that the method above breaks Thread confinement. Sound the alarms! We have an escaping variable! Exposing a public method that returns a ThreadLocal variable is dangerous because we have no control over what the other classes in the system will do with that variable once it is returned. In this project the problem turned out to be that one class adopted this ThreadLocal variable and turned it into a member variable. So sometimes it worked, and others it didn’t.

The best way to resolve this is to not let the enityManager escape. Encapsulate all access to this variable inside of a class that is responsible for ensuring thread safe access to the non thread safe variable. This solution is cumbersome and adds a layer of abstraction that isn’t core to the business logic but is certainly much better than transient assertion errors visible to the customer. Switching to a declarative transaction management implementation such as this one, Transaction Management Using Spring is a better solution in the long run.

Advertisement