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.

"In the original code the initalisation is expensive..." Might consider (or rule out) [Static holder singleton pattern](https://stackoverflow.com/questions/15019306/regarding-static-holder-singleton-pattern). — jmehrens, Aug 05 '22 at 14:11

jmehrens · Answer 1 · 2022-08-07T17:01:30.727

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.

"The example is racy because the read/load of etag is outside of the synchronized block". No, it is not racy. The read happens outside of the synchronized block, however the visibility guarantees of synchronized blocks guarantee that all threads that do not initialise etag will already see an initialised value after they exit the synchronized block. — Grodriguez, Aug 05 '22 at 10:37
"If the webserver were to create 200 instances of MutableFieldServlet (default number of threads in the thread pool) [...] It would not make any sense for a server to create 200 instances of the servlet even if there are 200 threads. Those are completely separate things. — Grodriguez, Aug 05 '22 at 10:40
"[...] Instead of getting the one true min etag value the requests are going to get various different min etag values". In the original code they would see the same value (a hash of the content, and not a timestamp which I only used to provide a minimal example that reproduces the warning). But anyway this is NOT the problem that the spotbugs warning is describing. The warning is about race conditions in *single-instance* cases (which is what any sane webserver would do) — Grodriguez, Aug 05 '22 at 10:41

Spotbugs "Mutable servlet field" warning

1 Answers1