[jira] Created: (STR-3192) Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?

Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
----------------------------------------------------------------------

                 Key: STR-3192
                 URL: https://issues.apache.org/struts/browse/STR-3192
             Project: Struts 1
          Issue Type: Improvement
    Affects Versions: 1.2.7
            Reporter: Juan Carlos Blanco Martinez
            Priority: Critical



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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=46581#action_46581 ]

Juan Carlos Blanco Martinez commented on STR-3192:
--------------------------------------------------

Is this 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?
In such a case it would make sense.

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=46880#action_46880 ]

Paul Benedict commented on STR-3192:
------------------------------------

Can you tell me why eager instantiation is preferred by you? Forgive me, but I don't understand what you're trying to achieve.

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

     [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Paul Benedict updated STR-3192:
-------------------------------

    Fix Version/s: Pending Review

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=46893#action_46893 ]

Juan Carlos Blanco Martinez commented on STR-3192:
--------------------------------------------------

Because, since the messages are needed from the very first request, it makes sense for me to eagerly load them rather than using the lazy instantiation, thus the performance (I know it doesn't make a huge difference but still) won't be affected by the synchronization.

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=46898#action_46898 ]

Paul Benedict commented on STR-3192:
------------------------------------

This class follows the "abstract toolkit" pattern in which it instantiates a factory, and then the factory instantiates the actual objects. Perhaps the pattern isn't totally necessary, but I don't know what can be done about this. MessageResourcesFactory is probably used many places. Refactoring would entail not referencing MessageResourcesFactory once the factory is obtained. Got any ideas on that one?

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

     [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Juan Carlos Blanco Martinez updated STR-3192:
---------------------------------------------

    Description:

Double-checked locking issue exists and one of the options to solve this is to use synchronization. In the class org.apache.struts.util.MessageResources, we use LAZY INSTANTIATION to instantiate MessageResourcesFactory:

public synchronized static MessageResources getMessageResources(String config) {

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

    return defaultFactory.createResources(config);
}

Why not to use:

EAGER INSTANTIATION

private static MessageResourcesFactory defaultFactory = MessageResourcesFactory.createFactory();

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.

  was:

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.

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> Double-checked locking issue exists and one of the options to solve this is to use synchronization. In the class org.apache.struts.util.MessageResources, we use LAZY INSTANTIATION to instantiate MessageResourcesFactory:
> public synchronized static MessageResources getMessageResources(String config) {
>     if (defaultFactory == null) {
>         defaultFactory = MessageResourcesFactory.createFactory();
>     }
>     return defaultFactory.createResources(config);
> }
> Why not to use:
> EAGER INSTANTIATION
> private static MessageResourcesFactory defaultFactory = MessageResourcesFactory.createFactory();
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=46893#action_46893 ]

Juan Carlos Blanco Martinez edited comment on STR-3192 at 10/6/09 1:32 AM:
---------------------------------------------------------------------------

Because, since the messages are needed from the very first request, it makes sense to me to eagerly load them rather than using the lazy instantiation, thus the performance (I know it doesn't make a huge difference but still) won't be affected by the synchronization.

      was (Author: jcblancomartinez):
    Because, since the messages are needed from the very first request, it makes sense for me to eagerly load them rather than using the lazy instantiation, thus the performance (I know it doesn't make a huge difference but still) won't be affected by the synchronization.
 

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> Double-checked locking issue exists and one of the options to solve this is to use synchronization. In the class org.apache.struts.util.MessageResources, we use LAZY INSTANTIATION to instantiate MessageResourcesFactory:
> public synchronized static MessageResources getMessageResources(String config) {
>     if (defaultFactory == null) {
>         defaultFactory = MessageResourcesFactory.createFactory();
>     }
>     return defaultFactory.createResources(config);
> }
> Why not to use:
> EAGER INSTANTIATION
> private static MessageResourcesFactory defaultFactory = MessageResourcesFactory.createFactory();
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=46899#action_46899 ]

Juan Carlos Blanco Martinez commented on STR-3192:
--------------------------------------------------

Sorry, I think you misunderstood me. I didn't mean to change the structure just the sort of instantiation of the MessageResourcesFactory from lazy to eager, so no need for the method getMessageResources to be declared as synchronized. This method is called many times so the performance could be slightly improved.

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> Double-checked locking issue exists and one of the options to solve this is to use synchronization. In the class org.apache.struts.util.MessageResources, we use LAZY INSTANTIATION to instantiate MessageResourcesFactory:
> public synchronized static MessageResources getMessageResources(String config) {
>     if (defaultFactory == null) {
>         defaultFactory = MessageResourcesFactory.createFactory();
>     }
>     return defaultFactory.createResources(config);
> }
> Why not to use:
> EAGER INSTANTIATION
> private static MessageResourcesFactory defaultFactory = MessageResourcesFactory.createFactory();
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/struts/browse/STR-3192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=47012#action_47012 ]

Paul Benedict commented on STR-3192:
------------------------------------

I don't see anyway around this.

> Why lazy instantiation of the MessageResourcesFactory in Struts 1.2.7?
> ----------------------------------------------------------------------
>
>                 Key: STR-3192
>                 URL: https://issues.apache.org/struts/browse/STR-3192
>             Project: Struts 1
>          Issue Type: Improvement
>    Affects Versions: 1.2.7
>            Reporter: Juan Carlos Blanco Martinez
>            Priority: Critical
>             Fix For: Pending Review
>
>
> Double-checked locking issue exists and one of the options to solve this is to use synchronization. In the class org.apache.struts.util.MessageResources, we use LAZY INSTANTIATION to instantiate MessageResourcesFactory:
> public synchronized static MessageResources getMessageResources(String config) {
>     if (defaultFactory == null) {
>         defaultFactory = MessageResourcesFactory.createFactory();
>     }
>     return defaultFactory.createResources(config);
> }
> Why not to use:
> EAGER INSTANTIATION
> private static MessageResourcesFactory defaultFactory = MessageResourcesFactory.createFactory();
> 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.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.