OFBiz › OFBiz - Dev

Issue with resetBshInterpreter

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

10 messages Options

Jacques Le Roux

Issue with resetBshInterpreter

Administrator

I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore

that we have an issue with this line
this.resetBshInterpreter(localContext);

That we find twice in ModelForm.java.

From stack trace it could related to immutable being introduced in the entity engine recently

---- cause ---------------------------------------------------------------------
Exception: java.lang.UnsupportedOperationException
Message: null
---- stack trace ---------------------------------------------------------------
java.lang.UnsupportedOperationException
java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)

I used this

Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
===================================================================
--- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
+++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
@@ -314,6 +314,13 @@
}
}

+ public void setMutable() {
+ if (!this.mutable) {
+ this.mutable = true;
+ this.fields = new HashMap<String, Object>(this.fields);
+ }
+ }
+
/**
* @return Returns the isFromEntitySync.
*/
@@ -1448,7 +1455,10 @@
// ---- Methods added to implement the Map interface: ----

public Object remove(Object key) {
- return this.fields.remove(key);
+ setMutable();
+ this.fields.remove(key);
+ setImmutable();
+ return this.fields;
}

public boolean containsKey(Object key) {

I can commit if you are ok with it

Jacques

Adrian Crum-3

Re: Issue with resetBshInterpreter

The GenericEntity instance was made immutable for a reason. Please do
not change its behavior.

-Adrian

On 5/8/2013 11:04 AM, Jacques Le Roux wrote:

> I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore
>
> that we have an issue with this line
> this.resetBshInterpreter(localContext);
>
> That we find twice in ModelForm.java.
>
> From stack trace it could related to immutable being introduced in the entity engine recently
>
> ---- cause ---------------------------------------------------------------------
> Exception: java.lang.UnsupportedOperationException
> Message: null
> ---- stack trace ---------------------------------------------------------------
> java.lang.UnsupportedOperationException
> java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
> org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
> org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
> org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)
>
>
> I used this
>
> Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
> ===================================================================
> --- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
> +++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
> @@ -314,6 +314,13 @@
> }
> }
>
> + public void setMutable() {
> + if (!this.mutable) {
> + this.mutable = true;
> + this.fields = new HashMap<String, Object>(this.fields);
> + }
> + }
> +
> /**
> * @return Returns the isFromEntitySync.
> */
> @@ -1448,7 +1455,10 @@
> // ---- Methods added to implement the Map interface: ----
>
> public Object remove(Object key) {
> - return this.fields.remove(key);
> + setMutable();
> + this.fields.remove(key);
> + setImmutable();
> + return this.fields;
> }
>
> public boolean containsKey(Object key) {
>
>
> I can commit if you are ok with it
>
> Jacques

Jacques Le Roux

Re: Issue with resetBshInterpreter

Administrator

A solution could be to rather do the same at MapContext.remove() level
But then we need to know if the collection is immutable or not to reset it after change.
And it seems there are no reliable ways to know if a Java collection is immutable or not, even using reflection.
We could introduce Guava com.google.common.collect.ImmutableCollection or rely on UnsupportedOperationException exceptions

What do you think?

Jacques

From: "Adrian Crum" <[hidden email]>

> The GenericEntity instance was made immutable for a reason. Please do
> not change its behavior.
>
> -Adrian
>
> On 5/8/2013 11:04 AM, Jacques Le Roux wrote:
>> I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore
>>
>> that we have an issue with this line
>> this.resetBshInterpreter(localContext);
>>
>> That we find twice in ModelForm.java.
>>
>> From stack trace it could related to immutable being introduced in the entity engine recently
>>
>> ---- cause ---------------------------------------------------------------------
>> Exception: java.lang.UnsupportedOperationException
>> Message: null
>> ---- stack trace ---------------------------------------------------------------
>> java.lang.UnsupportedOperationException
>> java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
>> org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
>> org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
>> org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)
>>
>>
>> I used this
>>
>> Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
>> ===================================================================
>> --- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
>> +++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
>> @@ -314,6 +314,13 @@
>> }
>> }
>>
>> + public void setMutable() {
>> + if (!this.mutable) {
>> + this.mutable = true;
>> + this.fields = new HashMap<String, Object>(this.fields);
>> + }
>> + }
>> +
>> /**
>> * @return Returns the isFromEntitySync.
>> */
>> @@ -1448,7 +1455,10 @@
>> // ---- Methods added to implement the Map interface: ----
>>
>> public Object remove(Object key) {
>> - return this.fields.remove(key);
>> + setMutable();
>> + this.fields.remove(key);
>> + setImmutable();
>> + return this.fields;
>> }
>>
>> public boolean containsKey(Object key) {
>>
>>
>> I can commit if you are ok with it
>>
>> Jacques
>

Adrian Crum-3

Re: Issue with resetBshInterpreter

Just use a try-catch block around the remove method call.

-Adrian

On 5/8/2013 12:30 PM, Jacques Le Roux wrote:

> A solution could be to rather do the same at MapContext.remove() level
> But then we need to know if the collection is immutable or not to reset it after change.
> And it seems there are no reliable ways to know if a Java collection is immutable or not, even using reflection.
> We could introduce Guava com.google.common.collect.ImmutableCollection or rely on UnsupportedOperationException exceptions
>
> What do you think?
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> The GenericEntity instance was made immutable for a reason. Please do
>> not change its behavior.
>>
>> -Adrian
>>
>> On 5/8/2013 11:04 AM, Jacques Le Roux wrote:
>>> I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore
>>>
>>> that we have an issue with this line
>>> this.resetBshInterpreter(localContext);
>>>
>>> That we find twice in ModelForm.java.
>>>
>>> From stack trace it could related to immutable being introduced in the entity engine recently
>>>
>>> ---- cause ---------------------------------------------------------------------
>>> Exception: java.lang.UnsupportedOperationException
>>> Message: null
>>> ---- stack trace ---------------------------------------------------------------
>>> java.lang.UnsupportedOperationException
>>> java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
>>> org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
>>> org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
>>> org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)
>>>
>>>
>>> I used this
>>>
>>> Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>> ===================================================================
>>> --- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
>>> +++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
>>> @@ -314,6 +314,13 @@
>>> }
>>> }
>>>
>>> + public void setMutable() {
>>> + if (!this.mutable) {
>>> + this.mutable = true;
>>> + this.fields = new HashMap<String, Object>(this.fields);
>>> + }
>>> + }
>>> +
>>> /**
>>> * @return Returns the isFromEntitySync.
>>> */
>>> @@ -1448,7 +1455,10 @@
>>> // ---- Methods added to implement the Map interface: ----
>>>
>>> public Object remove(Object key) {
>>> - return this.fields.remove(key);
>>> + setMutable();
>>> + this.fields.remove(key);
>>> + setImmutable();
>>> + return this.fields;
>>> }
>>>
>>> public boolean containsKey(Object key) {
>>>
>>>
>>> I can commit if you are ok with it
>>>
>>> Jacques

Jacques Le Roux

Re: Issue with resetBshInterpreter

Administrator

You mean that, right? It works but I'm unsure of the remove method call yuo talked about.

Index: framework/base/src/org/ofbiz/base/util/collections/MapContext.java
===================================================================
--- framework/base/src/org/ofbiz/base/util/collections/MapContext.java (revision 1480164)
+++ framework/base/src/org/ofbiz/base/util/collections/MapContext.java (working copy)
@@ -20,6 +20,7 @@

import java.util.Collection;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -251,8 +252,13 @@
*/
public V remove(Object key) {
// all write operations are local: only remove from the Map on the top of the stack
- Map<K, V> currentMap = this.stackList.get(0);
- return currentMap.remove(key);
+ Map<K, V> currentMap = this.stackList.get(0);
+ try {
+ return currentMap.remove(key);
+ } catch (UnsupportedOperationException e) {
+ Map<K, V> tempMap = new HashMap<K, V>(currentMap);
+ return (V) tempMap.remove(key);
+ }
}

Jacques

From: "Adrian Crum" <[hidden email]>

> Just use a try-catch block around the remove method call.
>
> -Adrian
>
> On 5/8/2013 12:30 PM, Jacques Le Roux wrote:
>> A solution could be to rather do the same at MapContext.remove() level
>> But then we need to know if the collection is immutable or not to reset it after change.
>> And it seems there are no reliable ways to know if a Java collection is immutable or not, even using reflection.
>> We could introduce Guava com.google.common.collect.ImmutableCollection or rely on UnsupportedOperationException exceptions
>>
>> What do you think?
>>
>> Jacques
>>
>> From: "Adrian Crum" <[hidden email]>
>>> The GenericEntity instance was made immutable for a reason. Please do
>>> not change its behavior.
>>>
>>> -Adrian
>>>
>>> On 5/8/2013 11:04 AM, Jacques Le Roux wrote:
>>>> I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore
>>>>
>>>> that we have an issue with this line
>>>> this.resetBshInterpreter(localContext);
>>>>
>>>> That we find twice in ModelForm.java.
>>>>
>>>> From stack trace it could related to immutable being introduced in the entity engine recently
>>>>
>>>> ---- cause ---------------------------------------------------------------------
>>>> Exception: java.lang.UnsupportedOperationException
>>>> Message: null
>>>> ---- stack trace ---------------------------------------------------------------
>>>> java.lang.UnsupportedOperationException
>>>> java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
>>>> org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
>>>> org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
>>>> org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)
>>>>
>>>>
>>>> I used this
>>>>
>>>> Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>> ===================================================================
>>>> --- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
>>>> +++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
>>>> @@ -314,6 +314,13 @@
>>>> }
>>>> }
>>>>
>>>> + public void setMutable() {
>>>> + if (!this.mutable) {
>>>> + this.mutable = true;
>>>> + this.fields = new HashMap<String, Object>(this.fields);
>>>> + }
>>>> + }
>>>> +
>>>> /**
>>>> * @return Returns the isFromEntitySync.
>>>> */
>>>> @@ -1448,7 +1455,10 @@
>>>> // ---- Methods added to implement the Map interface: ----
>>>>
>>>> public Object remove(Object key) {
>>>> - return this.fields.remove(key);
>>>> + setMutable();
>>>> + this.fields.remove(key);
>>>> + setImmutable();
>>>> + return this.fields;
>>>> }
>>>>
>>>> public boolean containsKey(Object key) {
>>>>
>>>>
>>>> I can commit if you are ok with it
>>>>
>>>> Jacques
>

Adrian Crum-3

Re: Issue with resetBshInterpreter

return currentMap.remove(key);

is the remove method call.

There is no point removing an element in a copy of the Map:

Map<K, V> currentMap = this.stackList.get(0);
try {
return currentMap.remove(key);
} catch (UnsupportedOperationException e) {
return (V) currentMap.get(key);
-OR-
return null;
}

But that simply hides the problem. The code calling this method should do the try-catch.

You said the problem is in one of the screen widgets - so that is where it should be fixed. Why is a widget removing elements from a Map? So, there is a design flaw in there somewhere.

Just let me know where in the screen widget code you encountered the problem. I caused it, so I should fix it.

-Adrian

On 5/8/2013 3:10 PM, Jacques Le Roux wrote:

> You mean that, right? It works but I'm unsure of the remove method call yuo talked about.
>
> Index: framework/base/src/org/ofbiz/base/util/collections/MapContext.java
> ===================================================================
> --- framework/base/src/org/ofbiz/base/util/collections/MapContext.java (revision 1480164)
> +++ framework/base/src/org/ofbiz/base/util/collections/MapContext.java (working copy)
> @@ -20,6 +20,7 @@
>
> import java.util.Collection;
> import java.util.Collections;
> +import java.util.HashMap;
> import java.util.List;
> import java.util.Locale;
> import java.util.Map;
> @@ -251,8 +252,13 @@
> */
> public V remove(Object key) {
> // all write operations are local: only remove from the Map on the top of the stack
> - Map<K, V> currentMap = this.stackList.get(0);
> - return currentMap.remove(key);
> + Map<K, V> currentMap = this.stackList.get(0);
> + try {
> + return currentMap.remove(key);
> + } catch (UnsupportedOperationException e) {
> + Map<K, V> tempMap = new HashMap<K, V>(currentMap);
> + return (V) tempMap.remove(key);
> + }
> }
>
> Jacques
>
>
> From: "Adrian Crum" <[hidden email]>
>> Just use a try-catch block around the remove method call.
>>
>> -Adrian
>>
>> On 5/8/2013 12:30 PM, Jacques Le Roux wrote:
>>> A solution could be to rather do the same at MapContext.remove() level
>>> But then we need to know if the collection is immutable or not to reset it after change.
>>> And it seems there are no reliable ways to know if a Java collection is immutable or not, even using reflection.
>>> We could introduce Guava com.google.common.collect.ImmutableCollection or rely on UnsupportedOperationException exceptions
>>>
>>> What do you think?
>>>
>>> Jacques
>>>
>>> From: "Adrian Crum" <[hidden email]>
>>>> The GenericEntity instance was made immutable for a reason. Please do
>>>> not change its behavior.
>>>>
>>>> -Adrian
>>>>
>>>> On 5/8/2013 11:04 AM, Jacques Le Roux wrote:
>>>>> I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore
>>>>>
>>>>> that we have an issue with this line
>>>>> this.resetBshInterpreter(localContext);
>>>>>
>>>>> That we find twice in ModelForm.java.
>>>>>
>>>>> From stack trace it could related to immutable being introduced in the entity engine recently
>>>>>
>>>>> ---- cause ---------------------------------------------------------------------
>>>>> Exception: java.lang.UnsupportedOperationException
>>>>> Message: null
>>>>> ---- stack trace ---------------------------------------------------------------
>>>>> java.lang.UnsupportedOperationException
>>>>> java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
>>>>> org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
>>>>> org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
>>>>> org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)
>>>>>
>>>>>
>>>>> I used this
>>>>>
>>>>> Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>>> ===================================================================
>>>>> --- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
>>>>> +++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
>>>>> @@ -314,6 +314,13 @@
>>>>> }
>>>>> }
>>>>>
>>>>> + public void setMutable() {
>>>>> + if (!this.mutable) {
>>>>> + this.mutable = true;
>>>>> + this.fields = new HashMap<String, Object>(this.fields);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> /**
>>>>> * @return Returns the isFromEntitySync.
>>>>> */
>>>>> @@ -1448,7 +1455,10 @@
>>>>> // ---- Methods added to implement the Map interface: ----
>>>>>
>>>>> public Object remove(Object key) {
>>>>> - return this.fields.remove(key);
>>>>> + setMutable();
>>>>> + this.fields.remove(key);
>>>>> + setImmutable();
>>>>> + return this.fields;
>>>>> }
>>>>>
>>>>> public boolean containsKey(Object key) {
>>>>>
>>>>>
>>>>> I can commit if you are ok with it
>>>>>
>>>>> Jacques

Jacques Le Roux

Re: Issue with resetBshInterpreter

Administrator

In reply to this post by Jacques Le Roux

I forgot one part I had before: to put back in currentMap
Though I wonder why it's not put back in this.stackList
I guess this comment explains it:
// all write operations are local: only remove from the Map on the top of the stack

So would be complete with

Index: framework/base/src/org/ofbiz/base/util/collections/MapContext.java
===================================================================
--- framework/base/src/org/ofbiz/base/util/collections/MapContext.java (revision 1480164)
+++ framework/base/src/org/ofbiz/base/util/collections/MapContext.java (working copy)
@@ -20,6 +20,7 @@

import java.util.Collection;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -251,8 +252,15 @@
*/
public V remove(Object key) {
// all write operations are local: only remove from the Map on the top of the stack
- Map<K, V> currentMap = this.stackList.get(0);
- return currentMap.remove(key);
+ Map<K, V> currentMap = this.stackList.get(0);
+ try {
+ return currentMap.remove(key);
+ } catch (UnsupportedOperationException e) {
+ Map<K, V> tempMap = new HashMap<K, V>(currentMap);
+ V value = tempMap.remove(key);
+ currentMap = Collections.unmodifiableMap(tempMap);
+ return value;
+ }
}

Jacques

----- Original Message -----
From: "Jacques Le Roux" <[hidden email]>
To: <[hidden email]>
Sent: Wednesday, May 08, 2013 4:10 PM
Subject: Re: Issue with resetBshInterpreter

You mean that, right? It works but I'm unsure of the remove method call yuo talked about.

Index: framework/base/src/org/ofbiz/base/util/collections/MapContext.java
===================================================================
--- framework/base/src/org/ofbiz/base/util/collections/MapContext.java (revision 1480164)
+++ framework/base/src/org/ofbiz/base/util/collections/MapContext.java (working copy)
@@ -20,6 +20,7 @@

import java.util.Collection;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -251,8 +252,13 @@
*/
public V remove(Object key) {
// all write operations are local: only remove from the Map on the top of the stack
- Map<K, V> currentMap = this.stackList.get(0);
- return currentMap.remove(key);
+ Map<K, V> currentMap = this.stackList.get(0);
+ try {
+ return currentMap.remove(key);
+ } catch (UnsupportedOperationException e) {
+ Map<K, V> tempMap = new HashMap<K, V>(currentMap);
+ return (V) tempMap.remove(key);
+ }
}

Jacques

From: "Adrian Crum" <[hidden email]>

Jacques Le Roux

Re: Issue with resetBshInterpreter

Administrator

In reply to this post by Adrian Crum-3

From: "Adrian Crum" <[hidden email]>

Yes I was thinking that too. But the problem is more general. As the stack I posted shows it (here again). It's in ModelForm, and not dependend of a specific screen widget

---- cause ---------------------------------------------------------------------
Exception: java.lang.UnsupportedOperationException
Message: null
---- stack trace ---------------------------------------------------------------
java.lang.UnsupportedOperationException
java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)

The screen and form are quite standard. It's rather related to this resetBshInterpreter call which we could maybe get rid off. But before considering anything like that I preferred a non intrusive way

Jacques

> You said the problem is in one of the screen widgets - so that is where it should be fixed. Why is a widget removing elements from a Map? So, there is a design flaw in there somewhere.
>
> Just let me know where in the screen widget code you encountered the problem. I caused it, so I should fix it.
>
> -Adrian
>
> On 5/8/2013 3:10 PM, Jacques Le Roux wrote:
>> You mean that, right? It works but I'm unsure of the remove method call yuo talked about.
>>
>> Index: framework/base/src/org/ofbiz/base/util/collections/MapContext.java
>> ===================================================================
>> --- framework/base/src/org/ofbiz/base/util/collections/MapContext.java (revision 1480164)
>> +++ framework/base/src/org/ofbiz/base/util/collections/MapContext.java (working copy)
>> @@ -20,6 +20,7 @@
>>
>> import java.util.Collection;
>> import java.util.Collections;
>> +import java.util.HashMap;
>> import java.util.List;
>> import java.util.Locale;
>> import java.util.Map;
>> @@ -251,8 +252,13 @@
>> */
>> public V remove(Object key) {
>> // all write operations are local: only remove from the Map on the top of the stack
>> - Map<K, V> currentMap = this.stackList.get(0);
>> - return currentMap.remove(key);
>> + Map<K, V> currentMap = this.stackList.get(0);
>> + try {
>> + return currentMap.remove(key);
>> + } catch (UnsupportedOperationException e) {
>> + Map<K, V> tempMap = new HashMap<K, V>(currentMap);
>> + return (V) tempMap.remove(key);
>> + }
>> }
>>
>> Jacques
>>
>>
>> From: "Adrian Crum" <[hidden email]>
>>> Just use a try-catch block around the remove method call.
>>>
>>> -Adrian
>>>
>>> On 5/8/2013 12:30 PM, Jacques Le Roux wrote:
>>>> A solution could be to rather do the same at MapContext.remove() level
>>>> But then we need to know if the collection is immutable or not to reset it after change.
>>>> And it seems there are no reliable ways to know if a Java collection is immutable or not, even using reflection.
>>>> We could introduce Guava com.google.common.collect.ImmutableCollection or rely on UnsupportedOperationException exceptions
>>>>
>>>> What do you think?
>>>>
>>>> Jacques
>>>>
>>>> From: "Adrian Crum" <[hidden email]>
>>>>> The GenericEntity instance was made immutable for a reason. Please do
>>>>> not change its behavior.
>>>>>
>>>>> -Adrian
>>>>>
>>>>> On 5/8/2013 11:04 AM, Jacques Le Roux wrote:
>>>>>> I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore
>>>>>>
>>>>>> that we have an issue with this line
>>>>>> this.resetBshInterpreter(localContext);
>>>>>>
>>>>>> That we find twice in ModelForm.java.
>>>>>>
>>>>>> From stack trace it could related to immutable being introduced in the entity engine recently
>>>>>>
>>>>>> ---- cause ---------------------------------------------------------------------
>>>>>> Exception: java.lang.UnsupportedOperationException
>>>>>> Message: null
>>>>>> ---- stack trace ---------------------------------------------------------------
>>>>>> java.lang.UnsupportedOperationException
>>>>>> java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
>>>>>> org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
>>>>>> org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
>>>>>> org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)
>>>>>>
>>>>>>
>>>>>> I used this
>>>>>>
>>>>>> Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>>>> ===================================================================
>>>>>> --- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
>>>>>> +++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
>>>>>> @@ -314,6 +314,13 @@
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> + public void setMutable() {
>>>>>> + if (!this.mutable) {
>>>>>> + this.mutable = true;
>>>>>> + this.fields = new HashMap<String, Object>(this.fields);
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> /**
>>>>>> * @return Returns the isFromEntitySync.
>>>>>> */
>>>>>> @@ -1448,7 +1455,10 @@
>>>>>> // ---- Methods added to implement the Map interface: ----
>>>>>>
>>>>>> public Object remove(Object key) {
>>>>>> - return this.fields.remove(key);
>>>>>> + setMutable();
>>>>>> + this.fields.remove(key);
>>>>>> + setImmutable();
>>>>>> + return this.fields;
>>>>>> }
>>>>>>
>>>>>> public boolean containsKey(Object key) {
>>>>>>
>>>>>>
>>>>>> I can commit if you are ok with it
>>>>>>
>>>>>> Jacques
>

Jacques Le Roux

Re: Issue with resetBshInterpreter

Administrator

In reply to this post by Adrian Crum-3

Note that this would not remove the bshInterpreter key/value pair which is what resetBshInterpreter tries to do

Anyway, we agree even resetBshInterpreter is weird...

Jacques

From: "Adrian Crum" <[hidden email]>

> return currentMap.remove(key);
>
> is the remove method call.
>
> There is no point removing an element in a copy of the Map:
>
> Map<K, V> currentMap = this.stackList.get(0);
> try {
> return currentMap.remove(key);
> } catch (UnsupportedOperationException e) {
> return (V) currentMap.get(key);
> -OR-
> return null;
> }
>
> But that simply hides the problem. The code calling this method should do the try-catch.
>
> You said the problem is in one of the screen widgets - so that is where it should be fixed. Why is a widget removing elements from a Map? So, there is a design flaw in there somewhere.
>
> Just let me know where in the screen widget code you encountered the problem. I caused it, so I should fix it.
>
> -Adrian
>
> On 5/8/2013 3:10 PM, Jacques Le Roux wrote:
>> You mean that, right? It works but I'm unsure of the remove method call yuo talked about.
>>
>> Index: framework/base/src/org/ofbiz/base/util/collections/MapContext.java
>> ===================================================================
>> --- framework/base/src/org/ofbiz/base/util/collections/MapContext.java (revision 1480164)
>> +++ framework/base/src/org/ofbiz/base/util/collections/MapContext.java (working copy)
>> @@ -20,6 +20,7 @@
>>
>> import java.util.Collection;
>> import java.util.Collections;
>> +import java.util.HashMap;
>> import java.util.List;
>> import java.util.Locale;
>> import java.util.Map;
>> @@ -251,8 +252,13 @@
>> */
>> public V remove(Object key) {
>> // all write operations are local: only remove from the Map on the top of the stack
>> - Map<K, V> currentMap = this.stackList.get(0);
>> - return currentMap.remove(key);
>> + Map<K, V> currentMap = this.stackList.get(0);
>> + try {
>> + return currentMap.remove(key);
>> + } catch (UnsupportedOperationException e) {
>> + Map<K, V> tempMap = new HashMap<K, V>(currentMap);
>> + return (V) tempMap.remove(key);
>> + }
>> }
>>
>> Jacques
>>
>>
>> From: "Adrian Crum" <[hidden email]>
>>> Just use a try-catch block around the remove method call.
>>>
>>> -Adrian
>>>
>>> On 5/8/2013 12:30 PM, Jacques Le Roux wrote:
>>>> A solution could be to rather do the same at MapContext.remove() level
>>>> But then we need to know if the collection is immutable or not to reset it after change.
>>>> And it seems there are no reliable ways to know if a Java collection is immutable or not, even using reflection.
>>>> We could introduce Guava com.google.common.collect.ImmutableCollection or rely on UnsupportedOperationException exceptions
>>>>
>>>> What do you think?
>>>>
>>>> Jacques
>>>>
>>>> From: "Adrian Crum" <[hidden email]>
>>>>> The GenericEntity instance was made immutable for a reason. Please do
>>>>> not change its behavior.
>>>>>
>>>>> -Adrian
>>>>>
>>>>> On 5/8/2013 11:04 AM, Jacques Le Roux wrote:
>>>>>> I just noticed at https://demo-trunk.ofbiz.apache.org/catalog/control/FindProductStore
>>>>>>
>>>>>> that we have an issue with this line
>>>>>> this.resetBshInterpreter(localContext);
>>>>>>
>>>>>> That we find twice in ModelForm.java.
>>>>>>
>>>>>> From stack trace it could related to immutable being introduced in the entity engine recently
>>>>>>
>>>>>> ---- cause ---------------------------------------------------------------------
>>>>>> Exception: java.lang.UnsupportedOperationException
>>>>>> Message: null
>>>>>> ---- stack trace ---------------------------------------------------------------
>>>>>> java.lang.UnsupportedOperationException
>>>>>> java.util.Collections$UnmodifiableMap.remove(Collections.java:1288)
>>>>>> org.ofbiz.entity.GenericEntity.remove(GenericEntity.java:1451)
>>>>>> org.ofbiz.base.util.collections.MapContext.remove(MapContext.java:255)
>>>>>> org.ofbiz.widget.form.ModelForm.resetBshInterpreter(ModelForm.java:2139)
>>>>>>
>>>>>>
>>>>>> I used this
>>>>>>
>>>>>> Index: framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>>>> ===================================================================
>>>>>> --- framework/entity/src/org/ofbiz/entity/GenericEntity.java (revision 1480164)
>>>>>> +++ framework/entity/src/org/ofbiz/entity/GenericEntity.java (working copy)
>>>>>> @@ -314,6 +314,13 @@
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> + public void setMutable() {
>>>>>> + if (!this.mutable) {
>>>>>> + this.mutable = true;
>>>>>> + this.fields = new HashMap<String, Object>(this.fields);
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> /**
>>>>>> * @return Returns the isFromEntitySync.
>>>>>> */
>>>>>> @@ -1448,7 +1455,10 @@
>>>>>> // ---- Methods added to implement the Map interface: ----
>>>>>>
>>>>>> public Object remove(Object key) {
>>>>>> - return this.fields.remove(key);
>>>>>> + setMutable();
>>>>>> + this.fields.remove(key);
>>>>>> + setImmutable();
>>>>>> + return this.fields;
>>>>>> }
>>>>>>
>>>>>> public boolean containsKey(Object key) {
>>>>>>
>>>>>>
>>>>>> I can commit if you are ok with it
>>>>>>
>>>>>> Jacques
>

Adrian Crum-3

Re: Issue with resetBshInterpreter

In reply to this post by Jacques Le Roux

I fixed this in revision 1480407.

It is very important that all developers understand that the recent
changes to the entity engine are intended to improve thread safety - and
they should not be short-circuited or defeated!

In this case, a form widget was trying to modify GenericValue instances
that were retrieved from the cache. So, we have a situation where
rendering code is changing the state of objects that should not be changed.

If you see any more exceptions like the one Jacques found, PLEASE fix
the exception properly - copy the immutable GenericEntity instance to a
Map that can be modified.

-Adrian

On 5/8/2013 11:04 AM, Jacques Le Roux wrote: