Re: svn commit: r769236 - /ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java

[jonesde]

It looks like there is some non-thread-safe code in this change. The  
"map" class field is set when the set operation is run and not as part  
of the XML definition and so would be changed by multiple threads  
using the same ServiceEcaSetField definition object.

The "map" field should be moved inside the "eval" method, preferable  
to just before where it is populated for the most localized use.

-David


On Apr 27, 2009, at 10:16 PM, [hidden email] wrote:

> Author: hansbak
> Date: Tue Apr 28 04:16:19 2009
> New Revision: 769236
>
> URL: http://svn.apache.org/viewvc?rev=769236&view=rev
> Log:
> intermittent error seems to be solved with this
>
> Modified:
>    ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> ServiceEcaSetField.java
>
> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> ServiceEcaSetField.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java?rev=769236&r1=769235&r2=769236&view=diff
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> ServiceEcaSetField.java (original)
> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> ServiceEcaSetField.java Tue Apr 28 04:16:19 2009
> @@ -36,12 +36,18 @@
>     public static final String module =  
> ServiceEcaSetField.class.getName();
>
>     protected String fieldName = null;
> +    protected String mapName = null;
> +    protected Map<String, Object> map = null;
>     protected String envName = null;
>     protected String value = null;
>     protected String format = null;
>
>     public ServiceEcaSetField(Element set) {
>         this.fieldName = set.getAttribute("field-name");
> +        if (UtilValidate.isNotEmpty(this.fieldName) &&  
> this.fieldName.indexOf('.') != -1) {
> +            this.mapName = fieldName.substring(0,  
> this.fieldName.indexOf('.'));
> +            this.fieldName =  
> this.fieldName.substring(this.fieldName.indexOf('.') +1);
> +        }
>         this.envName = set.getAttribute("env-name");
>         this.value = set.getAttribute("value");
>         this.format = set.getAttribute("format");
> @@ -61,34 +67,27 @@
>                 }
>             }
>             // TODO: rewrite using the ContextAccessor.java see hack  
> below to be able to use maps for email notifications
> -            // check if target is a map
> -            String mapName = null;
> -     Map<String, Object> map = null;
> -            if (UtilValidate.isNotEmpty(fieldName) &&  
> fieldName.indexOf('.') != -1) {
> -         mapName = fieldName.substring(0, fieldName.indexOf('.'));
> -         fieldName = fieldName.substring(fieldName.indexOf('.') +1);
> -         if (context.containsKey(mapName)) {
> -         map = (Map<String, Object>) context.get(mapName);
> -         } else {
> -         map = FastMap.newInstance();
> -         }
> -         }
> -
> +            // check if target is a map and create/get from contaxt
> +            if (UtilValidate.isNotEmpty(this.mapName) &&  
> context.containsKey(this.mapName)) {
> +                map = (Map<String, Object>) context.get(mapName);
> +            } else {
> +                map = FastMap.newInstance();
> +            }
>             // process the context changes
>             String newValue = null;
> -            if (UtilValidate.isNotEmpty(value)) {
> -                newValue = (String) this.format(value, context);
> -            } else if (UtilValidate.isNotEmpty(envName) &&  
> context.get(envName) != null) {
> -                newValue = (String) this.format((String)  
> context.get(envName), context);
> +            if (UtilValidate.isNotEmpty(this.value)) {
> +                newValue = (String) this.format(this.value, context);
> +            } else if (UtilValidate.isNotEmpty(envName) &&  
> context.get(this.envName) != null) {
> +                newValue = (String) this.format((String)  
> context.get(this.envName), context);
>             }
>
>             if (newValue != null) {
> -             if (map != null) {
> -             map.put(fieldName, newValue);
> -             context.put(mapName, map);
> -             } else {
> -             context.put(fieldName, newValue);
> -             }
> +                if (UtilValidate.isNotEmpty(this.mapName)) {
> +                    this.map.put(this.fieldName, newValue);
> +                    context.put(this.mapName, this.map);
> +                } else {
> +                    context.put(this.fieldName, newValue);
> +                }
>             }
>         }
>     }
>
>

[hans_bakker]

Thank you David,

these comments are highly appreciated and i will correct this shortly...

Thanks again.
Hans

On Tue, 2009-04-28 at 05:49 -0600, David E Jones wrote:

> It looks like there is some non-thread-safe code in this change. The  
> "map" class field is set when the set operation is run and not as part  
> of the XML definition and so would be changed by multiple threads  
> using the same ServiceEcaSetField definition object.
>
> The "map" field should be moved inside the "eval" method, preferable  
> to just before where it is populated for the most localized use.
>
> -David
>
>
> On Apr 27, 2009, at 10:16 PM, [hidden email] wrote:
>
> > Author: hansbak
> > Date: Tue Apr 28 04:16:19 2009
> > New Revision: 769236
> >
> > URL: http://svn.apache.org/viewvc?rev=769236&view=rev
> > Log:
> > intermittent error seems to be solved with this
> >
> > Modified:
> >    ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> > ServiceEcaSetField.java
> >
> > Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> > ServiceEcaSetField.java
> > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java?rev=769236&r1=769235&r2=769236&view=diff
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > ======================================================================
> > --- ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> > ServiceEcaSetField.java (original)
> > +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
> > ServiceEcaSetField.java Tue Apr 28 04:16:19 2009
> > @@ -36,12 +36,18 @@
> >     public static final String module =  
> > ServiceEcaSetField.class.getName();
> >
> >     protected String fieldName = null;
> > +    protected String mapName = null;
> > +    protected Map<String, Object> map = null;
> >     protected String envName = null;
> >     protected String value = null;
> >     protected String format = null;
> >
> >     public ServiceEcaSetField(Element set) {
> >         this.fieldName = set.getAttribute("field-name");
> > +        if (UtilValidate.isNotEmpty(this.fieldName) &&  
> > this.fieldName.indexOf('.') != -1) {
> > +            this.mapName = fieldName.substring(0,  
> > this.fieldName.indexOf('.'));
> > +            this.fieldName =  
> > this.fieldName.substring(this.fieldName.indexOf('.') +1);
> > +        }
> >         this.envName = set.getAttribute("env-name");
> >         this.value = set.getAttribute("value");
> >         this.format = set.getAttribute("format");
> > @@ -61,34 +67,27 @@
> >                 }
> >             }
> >             // TODO: rewrite using the ContextAccessor.java see hack  
> > below to be able to use maps for email notifications
> > -            // check if target is a map
> > -            String mapName = null;
> > -     Map<String, Object> map = null;
> > -            if (UtilValidate.isNotEmpty(fieldName) &&  
> > fieldName.indexOf('.') != -1) {
> > -         mapName = fieldName.substring(0, fieldName.indexOf('.'));
> > -         fieldName = fieldName.substring(fieldName.indexOf('.') +1);
> > -         if (context.containsKey(mapName)) {
> > -         map = (Map<String, Object>) context.get(mapName);
> > -         } else {
> > -         map = FastMap.newInstance();
> > -         }
> > -         }
> > -
> > +            // check if target is a map and create/get from contaxt
> > +            if (UtilValidate.isNotEmpty(this.mapName) &&  
> > context.containsKey(this.mapName)) {
> > +                map = (Map<String, Object>) context.get(mapName);
> > +            } else {
> > +                map = FastMap.newInstance();
> > +            }
> >             // process the context changes
> >             String newValue = null;
> > -            if (UtilValidate.isNotEmpty(value)) {
> > -                newValue = (String) this.format(value, context);
> > -            } else if (UtilValidate.isNotEmpty(envName) &&  
> > context.get(envName) != null) {
> > -                newValue = (String) this.format((String)  
> > context.get(envName), context);
> > +            if (UtilValidate.isNotEmpty(this.value)) {
> > +                newValue = (String) this.format(this.value, context);
> > +            } else if (UtilValidate.isNotEmpty(envName) &&  
> > context.get(this.envName) != null) {
> > +                newValue = (String) this.format((String)  
> > context.get(this.envName), context);
> >             }
> >
> >             if (newValue != null) {
> > -             if (map != null) {
> > -             map.put(fieldName, newValue);
> > -             context.put(mapName, map);
> > -             } else {
> > -             context.put(fieldName, newValue);
> > -             }
> > +                if (UtilValidate.isNotEmpty(this.mapName)) {
> > +                    this.map.put(this.fieldName, newValue);
> > +                    context.put(this.mapName, this.map);
> > +                } else {
> > +                    context.put(this.fieldName, newValue);
> > +                }
> >             }
> >         }
> >     }
> >
> >
>

--
Antwebsystems.com: Quality OFBiz services for competitive rates

[David E Jones-3]

Thanks for taking care of that Hans, your changes look good.

-David


On Apr 28, 2009, at 6:32 AM, Hans Bakker wrote:

> Thank you David,
>
> these comments are highly appreciated and i will correct this  
> shortly...
>
> Thanks again.
> Hans
>
> On Tue, 2009-04-28 at 05:49 -0600, David E Jones wrote:
>> It looks like there is some non-thread-safe code in this change. The
>> "map" class field is set when the set operation is run and not as  
>> part
>> of the XML definition and so would be changed by multiple threads
>> using the same ServiceEcaSetField definition object.
>>
>> The "map" field should be moved inside the "eval" method, preferable
>> to just before where it is populated for the most localized use.
>>
>> -David
>>
>>
>> On Apr 27, 2009, at 10:16 PM, [hidden email] wrote:
>>
>>> Author: hansbak
>>> Date: Tue Apr 28 04:16:19 2009
>>> New Revision: 769236
>>>
>>> URL: http://svn.apache.org/viewvc?rev=769236&view=rev
>>> Log:
>>> intermittent error seems to be solved with this
>>>
>>> Modified:
>>>   ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
>>> ServiceEcaSetField.java
>>>
>>> Modified: ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
>>> ServiceEcaSetField.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java?rev=769236&r1=769235&r2=769236&view=diff
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
>>> ServiceEcaSetField.java (original)
>>> +++ ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/
>>> ServiceEcaSetField.java Tue Apr 28 04:16:19 2009
>>> @@ -36,12 +36,18 @@
>>>    public static final String module =
>>> ServiceEcaSetField.class.getName();
>>>
>>>    protected String fieldName = null;
>>> +    protected String mapName = null;
>>> +    protected Map<String, Object> map = null;
>>>    protected String envName = null;
>>>    protected String value = null;
>>>    protected String format = null;
>>>
>>>    public ServiceEcaSetField(Element set) {
>>>        this.fieldName = set.getAttribute("field-name");
>>> +        if (UtilValidate.isNotEmpty(this.fieldName) &&
>>> this.fieldName.indexOf('.') != -1) {
>>> +            this.mapName = fieldName.substring(0,
>>> this.fieldName.indexOf('.'));
>>> +            this.fieldName =
>>> this.fieldName.substring(this.fieldName.indexOf('.') +1);
>>> +        }
>>>        this.envName = set.getAttribute("env-name");
>>>        this.value = set.getAttribute("value");
>>>        this.format = set.getAttribute("format");
>>> @@ -61,34 +67,27 @@
>>>                }
>>>            }
>>>            // TODO: rewrite using the ContextAccessor.java see hack
>>> below to be able to use maps for email notifications
>>> -            // check if target is a map
>>> -            String mapName = null;
>>> -     Map<String, Object> map = null;
>>> -            if (UtilValidate.isNotEmpty(fieldName) &&
>>> fieldName.indexOf('.') != -1) {
>>> -         mapName = fieldName.substring(0, fieldName.indexOf('.'));
>>> -         fieldName = fieldName.substring(fieldName.indexOf('.')  
>>> +1);
>>> -         if (context.containsKey(mapName)) {
>>> -         map = (Map<String, Object>) context.get(mapName);
>>> -         } else {
>>> -         map = FastMap.newInstance();
>>> -         }
>>> -         }
>>> -
>>> +            // check if target is a map and create/get from contaxt
>>> +            if (UtilValidate.isNotEmpty(this.mapName) &&
>>> context.containsKey(this.mapName)) {
>>> +                map = (Map<String, Object>) context.get(mapName);
>>> +            } else {
>>> +                map = FastMap.newInstance();
>>> +            }
>>>            // process the context changes
>>>            String newValue = null;
>>> -            if (UtilValidate.isNotEmpty(value)) {
>>> -                newValue = (String) this.format(value, context);
>>> -            } else if (UtilValidate.isNotEmpty(envName) &&
>>> context.get(envName) != null) {
>>> -                newValue = (String) this.format((String)
>>> context.get(envName), context);
>>> +            if (UtilValidate.isNotEmpty(this.value)) {
>>> +                newValue = (String) this.format(this.value,  
>>> context);
>>> +            } else if (UtilValidate.isNotEmpty(envName) &&
>>> context.get(this.envName) != null) {
>>> +                newValue = (String) this.format((String)
>>> context.get(this.envName), context);
>>>            }
>>>
>>>            if (newValue != null) {
>>> -             if (map != null) {
>>> -             map.put(fieldName, newValue);
>>> -             context.put(mapName, map);
>>> -             } else {
>>> -             context.put(fieldName, newValue);
>>> -             }
>>> +                if (UtilValidate.isNotEmpty(this.mapName)) {
>>> +                    this.map.put(this.fieldName, newValue);
>>> +                    context.put(this.mapName, this.map);
>>> +                } else {
>>> +                    context.put(this.fieldName, newValue);
>>> +                }
>>>            }
>>>        }
>>>    }
>>>
>>>
>>
> --
> Antwebsystems.com: Quality OFBiz services for competitive rates
>