Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 {
    }
}
share|improve this question
    
Seems reasonable to me. –  aroth Feb 19 '13 at 23:25
add comment

2 Answers 2

up vote 2 down vote accepted

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?

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

share|improve this answer
    
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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.