SimpleDateFormat is not so Simple

September 22, 2007

I came across this blog Another Code Treasure today. It provides an example of a poorly written method to validate a date and then provides a better alternative. Here is the better alternative provided in the blog:

private static SimpleDateFormat mySpanishDateFormatter =
             new SimpleDateFormat("ddMMMyyyy", new Locale("es"));
    
public static boolean isSpanishDateStringValid(String dateStr) {
        if(dateStr==null)
           return false;
        mySpanishDateFormatter.setLenient(false);
        try {
             mySpanishDateFormatter.parse(dateStr);
             return true;
        } catch (ParseException e) {
            return false;
        }
}

This code is fine as long as you have a single-threaded application. But what happens if you have a mutlti-threaded application? The answer is we don’t know because SimpleDateFormat is not thread-safe. The code above will likely cause an exception to be thrown. The fix is to store the SimpleDateFormat in a ThreadLocal variable. This way each Thread gets its’ own instance of the SimpleDateFormatter. Here is an example of a Thread-Safe implementation of the date validation code:

private final ThreadLocal threadLocalFormat = 
    new ThreadLocal();

private DateFormat getDateFormat() {
        if (threadLocalFormat.get() == null) {
            threadLocalFormat.set(new SimpleDateFormat("ddMMMyyyy", new Locale("es")));
        }
        return threadLocalFormat.get();
}

public static boolean isSpanishDateStringValid(String dateStr) {
        if(dateStr==null)
           return false;
        getDateFormat().setLenient(false);
        try {
             getDateFormat().parse(dateStr);
             return true;
        } catch (ParseException e) {
            return false;
        }
}

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

September 16, 2007

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.