Using ThreadLocal to bypass Servlet threadunsafety?

Question

I am debugging some old servlet with a lot of exceptions. No ConcurrentModificationExceptions thanks to a lot (too much) of synchronized keywords, but I still suspect servlet threadunsafety. I read this very interesting question about servlets and threadsafety, and think this example is a good base:

public class ExampleServlet extends HttpServlet {

    private Object thisIsNOTThreadSafe;

    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        Object thisIsThreadSafe;

        thisIsNOTThreadSafe = request.getParameter("foo"); // BAD!! Shared among all requests!
        thisIsThreadSafe = request.getParameter("foo"); // OK, this is thread safe.
    } 
}

Actually, the folks who coded my servlet seemed to be also aware about that but decided to bypass it by doing something like that:

public class ExampleServlet extends HttpServlet {

    private ThreadLocal<MyObject> thisIsMaybeThreadSafe = new ThreadLocal<MyObject>();;

    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        // we want to avoid having to use this parameter in every method
        thisIsMaybeThreadSafe.set((MyObject)getObjectInSesssion("foo"));
        doStuff(request, response);
    } 
}

And the code also contains things like

synchronized(request.getAttribute("foo")){
   doStuff(request, response);
}

I have a bad feeling about all this, and was looking for evidence that this is dangerous. Actually after reading the question NullPointerException when setting attribute, I got the feeling that something must be wrong because I get similar stacktraces like this one:

11:07:17,525 ERROR [com.mycompany.myproject.web.business.servlet.map.tree.MapServlet] Error processing AjaxTreeAccessRequest
java.lang.NullPointerException
    at org.apache.catalina.connector.Request.notifyAttributeAssigned(Request.java:1493)
    at org.apache.catalina.connector.Request.setAttribute(Request.java:1484)
    at org.apache.catalina.connector.RequestFacade.setAttribute(RequestFacade.java:539)
    at javax.servlet.ServletRequestWrapper.setAttribute(ServletRequestWrapper.java:244)
    at org.apache.myfaces.context.servlet.RequestMap.setAttribute(RequestMap.java:51)
    at org.apache.myfaces.util.AbstractAttributeMap.put(AbstractAttributeMap.java:108)
    at org.apache.myfaces.el.VariableResolverImpl.resolveVariable(VariableResolverImpl.java:304)
    at org.springframework.web.jsf.DelegatingVariableResolver.resolveOriginal(DelegatingVariableResolver.java:120)
    at org.springframework.web.jsf.DelegatingVariableResolver.resolveVariable(DelegatingVariableResolver.java:105)
    at com.mycompany.myproject.web.common.servlet.AbstractFacesServlet.getManagedBean(AbstractFacesServlet.java:67)
    at com.mycompany.myproject.web.business.servlet.map.tree.MapServlet.getSessionTreeBean(MapServlet.java:184)
    at com.mycompany.myproject.web.business.servlet.map.tree.AjaxTreeAccess.initRequest(AjaxTreeAccess.java:355)
    at com.mycompany.myproject.web.business.servlet.map.tree.AjaxTreeAccess.processRequest(AjaxTreeAccess.java:134)
    at com.mycompany.myproject.web.common.servlet.AbstractFacesServlet.handleRequest(AbstractFacesServlet.java:81)
    at org.springframework.web.context.support.HttpRequestHandlerServlet.service(HttpRequestHandlerServlet.java:63)
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:725)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:291)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at org.springframework.orm.hibernate3.support.OpenSessionInViewFilter.doFilterInternal(OpenSessionInViewFilter.java:198)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:76)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at com.mycompany.myproject.commun.presentation.jsf.OpenSynchronizedSessionInViewFilter.doFilterInternal(OpenSynchronizedSessionInViewFilter.java:58)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:76)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:265)
    at com.mycompany.myproject.web.common.filter.SwitchUserProcessingFilter.doFilter(SwitchUserProcessingFilter.java:66)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:275)
    at org.acegisecurity.ui.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:110)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:275)
    at org.acegisecurity.providers.anonymous.AnonymousProcessingFilter.doFilter(AnonymousProcessingFilter.java:125)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:275)
    at org.acegisecurity.wrapper.SecurityContextHolderAwareRequestFilter.doFilter(SecurityContextHolderAwareRequestFilter.java:81)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:275)
    at org.acegisecurity.ui.AbstractProcessingFilter.doFilter(AbstractProcessingFilter.java:229)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:275)
    at org.acegisecurity.ui.logout.LogoutFilter.doFilter(LogoutFilter.java:106)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:275)
    at org.acegisecurity.context.HttpSessionContextIntegrationFilter.doFilter(HttpSessionContextIntegrationFilter.java:286)
    at org.acegisecurity.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:275)
    at org.acegisecurity.util.FilterChainProxy.doFilter(FilterChainProxy.java:149)
    at org.acegisecurity.util.FilterToBeanProxy.doFilter(FilterToBeanProxy.java:98)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at com.mycompany.myproject.web.business.filter.UserBindingFilter.doFilter(UtilisateurCourantBindingFilter.java:55)
    at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:236)
    at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:167)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at com.mycompany.myproject.web.common.filter.SessionTimeoutFilter.doFilter(SessionTimeoutFilter.java:67)
    at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:236)
    at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:167)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at com.mycompany.myproject.web.common.filter.SessionLoginFilter.doFilter(SessionLoginFilter.java:56)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at com.mycompany.profiling.prof.filter.ProfContextFilter.doFilter(ProfContextFilter.java:26)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
    at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:219)
    at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:106)
    at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:503)
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:136)
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:79)
    at org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:610)
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:88)
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:526)
    at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1078)
    at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:655)
    at org.apache.coyote.http11.Http11NioProtocol$Http11ConnectionHandler.process(Http11NioProtocol.java:222)
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1566)
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.run(NioEndpoint.java:1523)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
    at java.lang.Thread.run(Thread.java:744)

I would rather get rid off all this ThreadLocal stuff, but the refactoring would be huge and risky in an old legacy code that none actually remembers how it works so I need serious advice.

FYI The whole app is in legacy spring-JSF code and some of the ThreadLocal variable are actually JSF session related beans. What should I do to check global thread safety of this app?

A general rule of thumb that served me well when I was working with servlets: never, under any circumstances, no, seriously never put any state in a servlet. I know this won't help you much since the damage is already done but that's what you should be aiming for while cleaning up this mess. And good luck. — biziclop, Mar 27 '15 at 10:56

score 2 · Answer 1 · answered Mar 26 '15 at 17:09

2

synchronized(request.getAttribute("foo")) is bad, because there can be no foo in request and you'll get NPE. Better use some dedicated lock object.
Regarding ThreadLocal usage -- it's fine if not overused. For code of size you posted it's ok, but I think real issue is in your real code-base and it's pretty impossible to give short helpful advice here except the one stating that you need write lots of unit tests (as simple as possible) for your real logic (and not the one concerned with Servlet API and concurrency) and then gradually refactor code base to more sane state.

Hope this helps :)

answered Mar 26 '15 at 17:09

Victor Sorokin

11,878
2
35
51

My start point for suspecting a concurrency issue was the question [NullPointerException when setting attribute?](http://stackoverflow.com/questions/13971099/nullpointerexception-when-setting-attribute), because I exactly have the same problem. There should be something wrong about how `ThreadLocal` are used but I will keep looking. Thanks. – Aldian Mar 27 '15 at 09:17
@Aldian You should add NPE stacktrace to your post then. `ThreadLocal` works fine if used correctly. – Victor Sorokin Mar 27 '15 at 09:45
Hmm, NPE occurs somewhere in library code, not in your code. You should try to debug NPE site, check what exactly is set to `null` and where it comes from (perhaps, from your code). – Victor Sorokin Mar 27 '15 at 11:03
If you look at the [affected library code](http://tiny.cc/o4u5vx), you see that line 1493 is `Object listeners[] = context.getApplicationEventListeners();`. NPE can only happen if context is null, meaning there is a multithreading issue. Whatever the param I am trying to set NPE will still happen – Aldian Mar 27 '15 at 14:47

gaRos · Answer 2 · 2015-03-27T09:46:29.913

This ThreadLocal for session handling seems to me a dangerous solution.

ThreadLocal will create a MyObject for every Thread that calls that class. Not every Client.

I'm pretty sure, that the servlet container will not create a new Thread for every incoming connection, and I'm also sure that you can't be sure that the same Thread will handle your request every time.

I might be wrong about how servlet container works (and tell me if I'm wrong) but if I were you I would create lots of concurrency tests to make sure that every client has a separate MyObject for their session.

If it's working that means under heavy load, every incoming connection will create one object. That's bad if you want to make your application scalable.

So what I think basically the problem is that in this code example the incoming connection is confused with the Thread that call the doGet. What do you think?

You should take a look at the question I linked, which explains how servlets work. For example, Servlets are NOT threadsafe. — Aldian, Mar 27 '15 at 09:07
@Aldian : You are right about Servlet thread safety, I will remove that from my answer! — gaRos, Mar 27 '15 at 09:22

Using ThreadLocal to bypass Servlet threadunsafety?

2 Answers2