Spotbugs "Mutable servlet field" warning

Question

Spotbugs is reporting a "Mutable servlet field" warning in this code (simplified test case):

import java.io.IOException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class MutableFieldServlet extends HttpServlet
{
    private String etag;

    @Override
    public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
    {
        synchronized (this)
        {
            if (etag == null)
                etag = "\"" + System.currentTimeMillis() + "\"";
        }

        resp.addHeader("ETag", etag);
        resp.getWriter().println("Hello world");
    }
}

The description says: "A web server generally only creates one instance of servlet or JSP class (i.e., treats the class as a Singleton), and will have multiple threads invoke methods on that instance to service multiple simultaneous requests. Thus, having a mutable instance field generally creates race conditions."

However in this case I would say that there is no race condition as all writes to the field happen within a synchronized block. Is this a false positive?

Edit: This is just a simplified test case that can be used to reproduce the SpotBugs warning. The actual code obviously does not use a timestamp as an etag.

Answer 1

The read/load of etag is outside of the synchronized block is odd but, maybe that is just a side effect of creating example code.

I think the part that gets tricky is:

A web server generally only creates one instance...

If the webserver were to create 200 instances of MutableFieldServlet (default number of threads in the thread pool). Instead of getting the one web classloader scope of the very first etag value may not be the same per request if the servlet container servers a different object from the pool.

Is this a false positive?

Per the ServletContext documentation:

In the case of a web application marked "distributed" in its deployment descriptor, there will be one context instance for each virtual machine. In this situation, the context cannot be used as a location to share global information (because the information won't be truly global). Use an external resource like a database instead.

I think the way to look at this and FindBugs is reporting a code pattern that is fragile. A comment and a suppress warning in the code might be the best action. Or think of it as FindBugs telling you to find a different code pattern with less smell. E.G. The holder pattern:

import java.io.IOException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class HolderPatternServlet extends HttpServlet {

    @Override
    public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
        resp.addHeader("ETag", Holder.ETAG);
        resp.getWriter().println("Hello world");
    }

    private static class Holder {
        public static final String ETAG = "\"" + System.currentTimeMillis() + "\"";
    }
}

Take a look at:

Servlets should not have mutable instance fields:

By contract, a servlet container creates one instance of each servlet and then a dedicated thread is attached to each new incoming HTTP request to process the request. So all threads share the servlet instances and by extension their instance fields. To prevent any misunderstanding and unexpected behavior at runtime, all servlet fields should then be either static and/or final, or simply removed.

This rule isn't about false positives it is about smell and fragile code patterns. Seems like the easy change would be to just switch to static and sync on a static lock object or the class object. That gets rid of any doubts as to how your code example may or may not work.