Why is this an issue?
The processHttpRequest method and methods called from it can be executed by multiple threads within the same servlet instance, and
state changes to the instance caused by these methods are, therefore, not threadsafe.
This is due to the servlet container creating only one instance of each servlet (javax.servlet.http.HttpServlet) and attaching a
dedicated thread to each incoming HTTP request. The same problem exists for org.apache.struts.action.Action but with different
methods.
To prevent unexpected behavior, avoiding mutable states in servlets is recommended. Mutable instance fields should either be refactored into local
variables or made immutable by declaring them final.
Exceptions
- Fields annotated with
@javax.inject.Inject, @javax.ejb.EJB,
@org.springframework.beans.factory.annotation.Autowired, @javax.annotation.Resource
- Fields initialized in
init() or init(ServletConfig config) methods
How to fix it
Code examples
Noncompliant code example
If the field is never modified, declare it final.
public class MyServlet extends HttpServlet {
String apiVersion = "0.9.1"; // Noncompliant, field changes are not thread-safe
}
Compliant solution
public class MyServlet extends HttpServlet {
final String apiVersion = "0.9.1"; // Compliant, field cannot be changed
}
Noncompliant code example
If a field is modified within instance methods, refactor it into a local variable. That variable can be passed as an argument to other functions if
needed.
public class MyServlet extends HttpServlet {
String userName; // Noncompliant, field changes are not thread-safe
@Override
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
userName = req.getParameter("userName"); // Different threads may write concurrently to userName
resp.getOutputStream().print(getGreeting());
}
public String getGreeting() { // Unpredictable value in field userName
return "Hello "+userName+"!";
}
}
Compliant solution
public class MyServlet extends HttpServlet {
@Override
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
String userName = req.getParameter("userName"); // Compliant, local variable instead instance field
resp.getOutputStream().print(getGreeting(userName));
}
public String getGreeting(String userName) { // Compliant, method argument instead instance field
return "Hello "+userName+"!";
}
}
Noncompliant code example
If you still prefer instance state over local variables, consider using ThreadLocal fields. These fields provide a separate instance
of their value for each thread.
public class MyServlet extends HttpServlet {
String userName; // Noncompliant, field changes are not thread-safe
@Override
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
userName = req.getParameter("userName"); // Different threads may write concurrently to userName
resp.getOutputStream().print(getGreeting());
}
public String getGreeting() { // Unpredictable value in field userName
return "Hello "+userName+"!";
}
}
Compliant solution
public class MyServlet extends HttpServlet {
final ThreadLocal<String> userName = new ThreadLocal<>(); // Compliant, field itself does not change
@Override
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
userName.set(req.getParameter("userName")); // Compliant, own value provided for every thread
resp.getOutputStream().print(getGreeting());
}
public String getGreeting() {
return "Hello "+userName.get()+"!"; // Compliant, own value provided for every thread
}
}
Noncompliant code example
If you have a use case that requires a shared instance state between threads, declare the corresponding fields as static to indicate
your intention and awareness that there is only one instance of the servlet. However, the static modifier alone does not ensure thread
safety. Make sure also to take countermeasures against possible race conditions.
public class MyServlet extends HttpServlet {
public long timestampLastRequest; // Noncompliant, field changes are not thread-safe
@Override
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
timestampLastRequest = System.currentTimeMillis();
resp.getOutputStream().print(timestampLastRequest); // Race condition
}
}
Compliant solution
public class MyServlet extends HttpServlet {
public static long timestampLastRequest; // Compliant, sharing state is our intention
@Override
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
long timestamp;
synchronized (this) {
timestamp = timestampLastRequest; // No race condition, synchronized get & set
timestampLastRequest = System.currentTimeMillis();
}
resp.getOutputStream().print(timestamp);
}
}
Resources
Articles & blog posts
