Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?

Question

Hi,

Since there is the Double-checked locking issue so we have to use synchronization to guarantee the concurrent access to the following method (org.apache.struts.util.MessageResources class) :

LAZY INSTANTIATION

public synchronized static MessageResources getMessageResources(String config) {

    if (defaultFactory == null) {
        defaultFactory = MessageResourcesFactory.createFactory();
    }

    return defaultFactory.createResources(config);
}

Why not to use:

EAGER INSTANTIATION

static {

    // Construct a new instance of the specified factory class
    try {
        if (clazz == null)
            clazz = RequestUtils.applicationClass(factoryClass);
        MessageResourcesFactory defaultFactory =
            (MessageResourcesFactory) clazz.newInstance();
    } catch (Throwable t) {
        LOG.error("MessageResourcesFactory.createFactory", t);
    }

}

And then:

public static MessageResources getMessageResources(String config) {

    return defaultFactory.createResources(config);
}

It would allow concurrent access to the method getMessageResources which at least in my case it may be called quite a few times.

The implications when not using synchronized are in here:

http://en.wikipedia.org/wiki/Double-checked_locking

Answer 1

The overhead incurred by synchronized methods on a modern JVM is so small as to become insignificant. Subsequent calls to the synchronized lazy-init factory method will be pretty much as fast as would calls to an unsynchronized eager-init method.

In terms of code, the lazy-init approach is simpler and easier to understand (in my opinion) than using a static initializer block. Also, when static init blocks fail, it can be very confusing to figure out where and why.

Answer 2

Unless there's any reason the MessageResourceFactory cannot be initialized early on (e.g., certain Servlet resources need to be initialized first), I think I like your solution better. I would guess that there's no reason for the Struts team to lazily load the factory other than that that's what the particular developer who worked on this is used to doing (people do tend to lazily load singletons even when it's not necessary).

Have you tried submitting a bug report, and proposing your solution?

Answer 3

Is MessageResourcesFactory thread-safe? The synchronized method protects both the setting of the field and the createResources method call. If it is thread-safe the locking could be moved to cover just setting the field and leave the method call outside the critical section.

Answer 4

I think it is a way for Struts to make sure that it works fine when in multi-thread mode, no matter if the person overriding org.apache.struts.util.MessageResources defines createResources(String configuration) as synchronized or not.