Synchronizing without blocking in a servlet filter

[Dennis Zandvliet]

Hi,

Within a servlet filter after every hour i want to generate new javascript from input from a database. While i'm updating i *don't* want the other requests to be blocked.

So i want to do something like this:

//notUpdating, use this flag to avoid other threads going into synchronized block


	   if (nextUpdate < lastModified && notUpdating) {
	   
            synchronized( getData.class ) {
                if( (nextUpdate < lastModified)) {
					notUpdating = false;
                                        data = getdata.retrieve();
					 
					 js_new = generateJavascript(data);
					 
					 js_old = js_new

					notUpdating = true;

                }
				
            }
			
	}

	write.out(js_old);

[Vijitha Kumara]

I'm not sure how "nextUpdate" and "lastModified" are calculated in the program, but shouldn't "lastModified" be updated inside block too?

[Steve Luke]

That code you put there would not necessarily be safe - if suffers from the Double Checked Locking idiom failure in the Java Memory Model (you can google for Java and Double Checked Locking to see why if fails, usually discussed in the context of Singletons). On the other hand, though, I am not sure it is at all necessary.

If you have a task you want to do every x period of time, why not setup a scheduled task to do it at its own pace? For example, have a ScheduledThreadPoolExecutor with a Runnable that makes the call when it needs to. The Runnable builds a local String with the proper content and stores it in the ServletContext when it is done. The Servlet Request Filter simply pulls the current copy from the ServletContext with no need to worry about synchronization or incomplete data. If I did this I would probably start/stop the thread/executor from a ServletContextListener rather than the Filter to be sure it started up as soon as the web app does, and stops when the web app does.

public class UpdateJSCodeTask implements ServletContextListener, Runnable
{  //Executor to run task 1/hour, and Future to keep track of the running task
    private ScheduledExecutorService taskRunner;
    private ScheduledFuture taskTracker;
 
    //The ServletContext will be used to store the javascript String
    private ServletContext thisContext;

    private static final long JS_UPDATE_SCHEDULE = 60; //in minutes

    /* Part of the ServletContectListener contract, called when the Context is created
     * and used to store the ServletContext and start the task running.
     */
    public void contextInitialized(ServletContextEvent sce)
    {
        this.thisContext = sce.getServletContext();
        this.taskRunner = Exeutors.newSingleThreadScheduledExecutor();
        this.taskTracker = this.taskRunner.scheduleAtFixedRate(this, 0, JS_UPDATE_SCHEDULE, TimeUnit.MINUTES); //start now, run 1/hour
    }

    /* Part of the ServletContectListener contract, called when the Context is destroyed
     * and used to stop the task and kill the executor.
     */
    public void contextDestroyed(ServletContextEvent sce)
    {
        this.taskTracker.cancel();
        this.taskRunner.shutdownNow();
    }

    /* Part of the Runnable contract, called periodically by the Executor
     * and used to generate the javascript String and store it where
     * requests can get ahold of it.
     */
    public void run()
    {
        String new_js = getData.retrieve();
        this.thisContext.setAttribute("current_js", new_js);
    }

    //... other stuff... like maybe the code for retrieving the javascript code (getData.retrieve())
}

** Note this is example untested code, not even compiled. I am just showing it as an example. **

[Dennis Zandvliet]

Vijitha Kumara wrote:I'm not sure how "nextUpdate" and "lastModified" are calculated in the program, but shouldn't "lastModified" be updated inside block too?

Yes you're right, i just quickly made up some code to illustrate my problem.
lastModified is the last time the data has been updated, and nextupdate = lastModified + refreshtime

[Dennis Zandvliet]

Steve Luke wrote:
If you have a task you want to do every x period of time, why not setup a scheduled task to do it at its own pace?

I didn't want to make it to complicated with threading inside a filter etc.

I just wanted to use a simple update scheme: within a request use the old data unless it's time to refresh.

something similiar like this:

final long lastModified = file.lastModified();

if (xmlLastModified >= lastModified) {
dosomething ;
}

[Steve Luke]

Dennis Zandvliet wrote:
I didn't want to make it to complicated with threading inside a filter etc.

Unfortunately, synchronizing data between threads isn't easy, and can even be complicated at times. I am all for keeping things simple, but simplifying too much often leads to errors (and if you search for double checked locking you will see how complicated code can get to avoid the errors inherent in using it). I am not saying you need to use the tool I described earlier - is just an example of an 'aggressive' value-caching system which avoids synchronization issues (low response latency, simple request code).

I just wanted to use a simple update scheme: within a request use the old data unless it's time to refresh.

Then you should stick with straight up synchronization. Double checked locking is broken in Java and is difficult/impossible to fix, and often doesn't fit a real need (i.e. adds complication and chance for errors, but gains very little measurable difference). If you wanted to lazily update the data without causing (unnecessary) latency to the user and don't want to generate a new thread then use synchronized calls after the FilterChain#doFilter(...) call in the Servlet Filter. Then, each request will respond quickly with the last stored data and when the time for new data comes up then the cost of getting it is pushed to after the user getting their response. The down-side: all requests will use stale data. While one request updates data other requests will have to wait for it (but the response is already at the user - this amounts to fewer Threads available to the Servlet Container's Thread Pool for handling future requests). The up-side: Even the request which generates the new data is fast to respond. You safely generate new data just once per cycle. You don't have to worry about out-of-order writes, out-of-sequence calling, or code-moving causing errors. You will have to way the ups and the downs yourself (for example if it takes a real long time to update the data and you have a small thread pool to run requests in and/or have a high load on the server then this might not be best).

    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
    {
        //...
        write.out(js_old);  //even if it is a bit stale...

       chain.doFilter(request, response);

        //... maybe flush response, dunno ....

        synchronized(getData.class)
        {
            // ... get currentTime ...

            if (nextUpdate < currentTime)
            {
                data = getData.retrieve();
                js_new = generateJavascript(data);
                js_old = js_new;
                nextUpdate = currentTime + refreshTime;
            }
        }
    }
}

[Dennis Zandvliet]

Steve Luke wrote:
You will have to way the ups and the downs yourself (for example if it takes a real long time to update the data and you have a small thread pool to run requests in and/or have a high load on the server then this might not be best).

Thank you for your response. I will stick with this. It's not a problem that the data is stale, because it's just a user list, which will be updated outside the app infrequently.

[Dennis Zandvliet]

Steve Luke wrote:

    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
    {
        //...
        write.out(js_old);  //even if it is a bit stale...

       chain.doFilter(request, response);

        //... maybe flush response, dunno ....

        synchronized(getData.class)
        {
            // ... get currentTime ...

            if (nextUpdate < currentTime)
            {
                data = getData.retrieve();
                js_new = generateJavascript(data);
                js_old = js_new;
                nextUpdate = currentTime + refreshTime;
            }
        }
    }
}

One more question how do you properly declare js_new and js_old with respect to, scope, synchronization and type, StringBuilder or StringBuffer, and this similar discussion in mind?

[Steve Luke]

Dennis Zandvliet wrote:One more question how do you properly declare js_new and js_old with respect to, scope, synchronization and type, StringBuilder or StringBuffer, and this similar discussion in mind?

First let's talk about type:
1) I 100% disagree with the conclusions the blog post you linked to makes. The Vector class is legacy and should not be used - in ANY situation unless your code must run in a 1.0 JVM (in which case it is the ONLY choice to make). If you need a synchronized version of a List, use the Collections.synchronizedList() or choose one of the better designed Collection implementations in the java.util.concurrent package (Java 1.5+). The example Producer/Consumer code from the post is a bit contrived and still has synchronization problems when you use Vector (a single call in Vector is synchronized which prevents multiple consumers from attempting to remove the same object or from removing incorrectly indexed object, but iterative calls aren't - to get iterative consistency you need to synchronize the entire loop), and really represents a poor choice of collections (better suited with a Queue, rather than a List, and perhaps a BlockingQueue for what the code is supposed to do.) I guess the conclusion here is that I think the blog post is both irrelevant and incorrect.

2) For js_old and js_new you have 3 choices of type, String, StringBuilder, and StringBuffer. String is immutable - it can't be changed, so it is inherently thread safe. Since js_old is shared between multiple threads I would choose to make it a String. js_new is the results of some data manipulation that builds a CharacterSequence. It is being used only locally to store the results before being assigned to the shared reference. It makes sense to make js_new the same type as js_old, but it doesn't really matter.

Now let's talk scope
1) js_old has to be shared between multiple threads and multiple method calls. This means the scope should be larger than the method - an instance variable, or larger. My personal preference is to store data like this in the ServletContext rather than as instance variables because you have no control over the life-cycle of the Servlet / Servlet Filter, which means that the Filter may be taken out of service and replaced by another (or you could have multiple different instances of the Filter running in different JVMs) without you knowing about it. But that is more of a supposed concern, it isn't real until you get into load balancing / multi-server systems. So my choice for js_old would either be private instance variable or stored in the Servlet Context.

2) js_new is used only in the synchronized block to temporarily hold the value which gets assigned to js_old. As such, it should be created in the smallest scope as possible - inside the synchronized block at least, inside the if statement at best.

3) The method generateJavascript() is really the only code that needs to worry about creating a mutable character sequence. So it should use a StringBuffer or StringBuilder. It should be created local to the method, which means thread safety is not an issue, which lends itself to using a StringBuilder (if you know you are working in a Java 1.5+ environment, or StringBuffer if you don't know the JVM version.)

[Dennis Zandvliet]

After implementing the simple solution I know decided to go for the Threaded solution, so I've some more questions. :)

-Why not use a timer instead of a scheduler since we only create one thread?
-How do you do a retry if getting the data from the datasource fails? Do you that inside the thread or recreate the thread?
-I'm storing all the retrieved data in a map, what's the best strategy for repopulating?
a) Reuse the map: before repopulating the map, first clear all the entries. Effectively I think this means blocking the app again
b) Or creating a new instance of a map, populate this and at the end of the population update the global reference to this map. cons: need twice as much memory. (and the map is pretty large about 5000 user profiles)

Steve Luke wrote: If I did this I would probably start/stop the thread/executor from a ServletContextListener rather than the Filter to be sure it started up as soon as the web app does, and stops when the web app does.

Retrieving the data takes about 10 seconds, how will i be sure that the data is ready when the first request arrives at the filter?

[Kumar Raja]

This post is a really nice post for learning ,though, I'm taking time to grasp the whole stuff and I'm book marking it.

Thanks Dennis, for bringing up this post.