# Correct implementation for background task execution in web application?

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.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 {
}
}

-
Seems reasonable to me. –  aroth Feb 19 '13 at 23:25

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?

-
Excellent point (regarding static). Thanks! –  Chris Knight Feb 20 '13 at 16:18
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.

-
Thanks @Palacsint. Lack of synchonization was something I spotted myself after posting this and I have already added synchonization to the getting and setting of the cached data, so good spot. Also agree with your second point, but would make them static final constants and probably rename period, though not sure what's best: 'ONE_HOUR' possibly. Anyway, thanks for taking the time to look at my code. –  Chris Knight Feb 23 '13 at 21:21