You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by ha...@apache.org on 2009/04/28 06:16:20 UTC
svn commit: r769236 -
/ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java
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);
+ }
}
}
}
Re: svn commit: r769236 - /ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java
Posted by David E Jones <da...@hotwaxmedia.com>.
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, hansbak@apache.org 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
>
Re: svn commit: r769236 -
/ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java
Posted by Hans Bakker <ma...@antwebsystems.com>.
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, hansbak@apache.org 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
Re: svn commit: r769236 - /ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java
Posted by David E Jones <jo...@apache.org>.
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, hansbak@apache.org 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);
> + }
> }
> }
> }
>
>
Re: svn commit: r769236 - /ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java
Posted by David E Jones <jo...@apache.org>.
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, hansbak@apache.org 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);
> + }
> }
> }
> }
>
>