views:

211

answers:

2

Let's say I have a class in my web app called class "Foo". It has an initialise() method that is called when the bean is created using Spring. The initialise() method then tries to load an external service and assign it to a field. If the service could not be contacted, the field will be set to null.

private Service service;

public void initialise() {
    // load external service
    // set field to the loaded service if contacted
    // set to field to null if service could not be contacted
}

When someone calls the method get() on the class "Foo" the service will be invoked if it was started in the initialise() method. If the field for the service is null, I want to try and load the external service.

public String get() {
    if (service == null) {
        // try and load the service again
    }
    // perform operation on the service is service is not null
}

Is it possible that I may have sync issues if I would do something like this?

A: 

Yes, you will have a sync problem.

Lets assume you have single servlet:

public class FooServlet extends HttpServlet {

    private MyBean myBean;

    public void init() {
        myBean = (MyBean) WebApplicationContextUtils.
            getRequiredWebApplicationContext(getServletContext()).getBean("myBean");
    }

    public void doGet(HttpRequest request, HttpResponse response) {
        String string = myBean.get();
        ....
    }

}

class MyBean {
    public String get() {
        if (service == null) {
            // try and load the service again
        }
        // perform operation on the service is service is not null
    }
}

And your bean definition looks like:

<bean id="myBean" class="com.foo.MyBean" init-method="initialise" />

The problem is that your servlet instance is used by multiple request threads. Hence, the code block guarded by service == null may be entered by multiple threads.

The best fix (avoiding double-checked-locking etc) is:

class MyBean {
    public synchronized String get() {
        if (service == null) {
            // try and load the service again
        }
        // perform operation on the service is service is not null
    }
}

Hope this makes sense. Drop a comment if not.

toolkit
thanks for that. however, would it be resolved simply by making the method synchronized? or is there more to it then that?
digiarnie
A: 

toolkit's answer is correct. To solve the problem, just declare your Foo's initialise() method to be synchronized. You could refactor Foo as:

private Service service;

public synchronized void initialise() {
    if (service == null) {
        // load external service
        // set field to the loaded service if contacted
    }
}

public String get() {
    if (service == null) {            
        initialise(); // try and load the service again
    }
    // perform operation on the service is service is not null
}
Dov Wasserman
thanks for that! great idea with the refactoring ;)
digiarnie
That's a form of double-checked locking, and thus: 1. This only works with Java 5 or later. 2. "service" should be declared volatile.
Chris Jester-Young
Reference: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
Chris Jester-Young
thanks for that reference. it was really interesting. (and I'm working with Java 5)
digiarnie