Correct implementation for background task execution in web application?

Question

I have a simple servlet which merely serves some cached data back to the user. I also have a background thread which runs at fixed intervals to refresh the cached data. Is this a reasonable implementation?

@WebServlet("/CachingServlet")
public class CachingServlet extends HttpServlet {
    private static final long serialVersionUID = 1L;
    private static ScheduledExecutorService backgroundExecutor;
    private static String cachedData = "";

    public CachingServlet() {
        super();
    }

    @Override
    public void destroy() {
        backgroundExecutor.shutdown();
        super.destroy();
    }

    @Override
    public void init() throws ServletException {
        backgroundExecutor = Executors.newSingleThreadScheduledExecutor();
        backgroundExecutor.scheduleAtFixedRate(new Runnable() {
            @Override
            public void run() {
                //implemention of fetchNewData() omitted for clarity
                cachedData = fetchNewData();  
            }
        }, 0, 1, TimeUnit.HOURS);
        super.init();
    }

    protected void doGet(HttpServletRequest request, HttpServletResponse response) 
     throws ServletException, IOException   {
        PrintWriter out = response.getWriter();
        out.println(cachedData);
    }

    protected void doPost(HttpServletRequest request, HttpServletResponse response) 
     throws ServletException, IOException {
    }
}

Answer 1

you needn't call super.init(); according to docs, it is a NOP . but as a rule calling initializers of some superclass at the beginning of the overriding method will save you headache in the future.

backgroundExecutor shouldn't be static. suppose you have multiple instances of this servlet in an application. destroying any one of those servlets shuts down the last created updater. what happens to the rest?

Answer 2

1. There is no guarantee that the doGet() method will see the modified value of cachedData. You should synchronize the access of this field (both read and write) since the container calls the doGet() on separate threads than the backgroundExecutor's thread. An AtomicReference seems the easiest solution here.

   private final AtomicReference<String> cachedDataRef = 
       new AtomicReference<String>("");
   
   ...
   
   
   public void run() {
       // implemention of fetchNewData() omitted for clarity
       final String newData = fetchNewData();
       cachedDataRef.set(newData);
   }
   
   ...
   
   out.println(cachedDataRef.get());
   

2. I'd create two local variables for the 0 and 1 magic numbers:

   int initialDelay = 0;
   int period = 1;
   

   Reference: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler:

   Put the result of the expression, or parts of the expression, in a temporary variable with a name that explains the purpose.

   And Clean Code by Robert C. Martin, G19: Use Explanatory Variables.