You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Hans Bakker <ma...@antwebsystems.com> on 2009/04/28 14:32:35 UTC
Re: svn commit: r769236 -
/ofbiz/trunk/framework/service/src/org/ofbiz/service/eca/ServiceEcaSetField.java
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 <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
>