Finally, Fixed

Earlier, I mentioned a threading bug that I had introduced. I had tripped up on a variation of an antipattern that I’m usually alert to, so in the interest of public shaming and lessons learned, I’ll tell you about it.

The short version is that if you have to temporarily change the value of a ThreadLocal a) keep the original value and b) reset the ThreadLocal to the original value in a finally statement. Why might you want to do this? An example would be changing the security context so you can “do some stuff” as another user.

// part a
ThreadContext oldContext = MyThreadContextHelper.setContext(newContext);
try {
  // do some stuff in the new context....
} finally {
  // part b
  MyThreadContextHelper.setContext(oldContext);
}

Part b (use a try/finally) is what I usually see violated. E.g., I sometimes see this in a javax.servlet.Filter:

doSomeSetup();
chain.doFilter(request, response);
doSomeTearDown();

The problem is that you’re not going to hit the tear down if the chain exits with an Exception. So it should be:

doSomeSetup();
try {
  chain.doFilter(request, response);
} finally {
  doSomeTearDown();
}

In this version of the problem, it has nothing to do with Threads. You might be trying to wrap the chain.doFilter() with some logging or timing statements.

In my particular bug, I was being lazy about keeping the old context. I had assumed there was no initial context, so when I reset the context in the finally, I just set it to null.

// Lazy and wrong
MyThreadContextHelper.setContext(newContext);
try {
  // do some stuff in the new context....
} finally {
  MyThreadContextHelper.setContext(null);
}

Which I changed to look like the first code snippet.

p.s Greg, it’s fixed now.

It's only fair to share...
Share on FacebookGoogle+Tweet about this on TwitterShare on LinkedIn

Leave a Reply