You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@hlmksw.com> on 2010/05/19 19:49:12 UTC
Developers Note: Screen Widget Model Classes
Just a friendly reminder to the OFBiz developers and committers: The
screen widget model classes should not have their state altered by
renderers. That is not thread-safe and it results in undesirable behavior.
I know it's easy to forget - I just deprecated some methods that I wrote
where I made that mistake. :-)
It might help to think of the widget model classes as being immutable
(though technically they aren't). Once they are constructed and put in
the cache, they shouldn't be changed.
If you see code where a renderer calls a model's renderXxxx method, and
inside that method the model's fields are being changed, then that is a
no-no.
-Adrian
Re: Developers Note: Screen Widget Model Classes
Posted by Marc Morin <ma...@emforium.com>.
With a well behaved model system, no processing state should be stored in the model object.
The renderer uses a modified visitor pattern, (should be real pattern), then the state for the rendering instance can and should the be store in the renderer/visitor instance.
----- "Adrian Crum" <ad...@yahoo.com> wrote:
> That could be done too. I don't see where it would be safer. You can
> push/pop a state object in the renderer.
>
> -Adrian
>
> --- On Fri, 5/28/10, Harmeet Bedi <ha...@gmail.com> wrote:
> > > The state data doesn't need to
> > be stored in the model objects ... can
> > be stored just as easily in the renderer.
> >
> > I think safer practice should be to store state in context
> > not renderer.
> > So push new map on context, call sub-widget rendering and
> > pop map on
> > child widget render finish.
> >
> > Renderer instance is single per screen render. I think of
> > it as
> > something like global context. So would need to clean state
> > after use.
> >
> > Context(MapStack) may be better suited for this ?
> >
> > Harmeet
> >
> > On 29/05/10 4:23 AM, Adrian Crum wrote:
> > > --- On Fri, 5/28/10, Harmeet Bedi<ha...@gmail.com>
> > wrote:
> > >> Forms/Screens are specified in XML with a key -
> > based on
> > >> xml file and name of element. XML Specifications
> > is
> > >> converted into Object model using model classes.
> > These
> > >> objects are then used to render.
> > >>
> > >> Since model classes are cached and shared, the
> > model must
> > >> accurately represent the XML specification at the
> > start of
> > >> rendering.
> > >>
> > >> So a way could be immutable classes
> > >> Another way could be to return cloned copy of
> > cached object
> > >> at the start of rendering. If you do this it does
> > not matter
> > >> if anyone makes a mistake in the dozens of model
> > classes
> > >> wrt. cloned. (which many people may do over life
> > of project
> > >> because it is human to make stupid mistakes)
> > >>
> > >> It may also be worth asking why cache. To me
> > answer is
> > >> performance due to cost of XML parsing and
> > conversion from
> > >> XML to object model.
> > >> Creating Flexible string expander instance
> > variables in the
> > >> model classes may also be a cost but it is not due
> > to
> > >> FlexibleStringExpander cache.
> > >
> > > Actually, there is no reason to create
> > FlexibleStringExpander instances - you can call the static
> > expandString method with very little performance loss.
> > >
> > >> Wondering if the following proposal makes sense:
> > >> - Generate Model Classes from XSD using something
> > like
> > >> JAXB. These would not have any flexible string
> > expander
> > >> instance variables.
> > >> - Cache the key (resource+name) to Model for
> > lookup of
> > >> screens and forms
> > >> - In cache lookup. Clone copy of Model root. Wrap
> > the
> > >> cloned copy with a dynamic proxy. Dynamic proxy
> > also stores
> > >> reference to context.
> > >> - Getters from the dynamic proxy return values by
> > applying
> > >> FlexibleStringExpander.getInstance(..) on the
> > value obtained
> > >> from underlying cloned model.
> > >> - Change Render classes to use these generated
> > JAXB
> > >> objects.
> > >
> > > That would eliminate the unmarshalling code.
> > >
> > >> Advantage could be
> > >> - Most of the handwritten code for Model Classes
> > goes
> > >> away.
> > >
> > > I don't know about "most." From my perspective, the
> > unmarshalling code is a small percentage of the screen
> > widget code (basically just the class constructors).
> > >
> > > If we used the builder pattern, we could unmarshall
> > the XML files by using a builder class that builds the model
> > objects. Builder classes could build model objects from XML
> > files, or from another source - like a database, a CMS, or
> > another file format.
> > >
> > >> - Model classes and XML specification are tied
> > together and
> > >> specified only by XSD.
> > >> - Rendering presents a pipeline. Model classes can
> > be
> > >> modified by renderer if needed as the pipeline
> > proceeds from
> > >> start of rendering till end.
> > >
> > > The state data doesn't need to be stored in the model
> > objects - it's only stored there now because the developer
> > who put it there didn't know any better. It can be stored
> > just as easily in the renderer. There is no need to clone
> > model objects.
> > >
> > >> - Dynamic proxies are small. Mostly standard
> > conversion
> > >> (apply FlexibleStringExpander::expand) but it
> > could be the
> > >> spot to add any additional behavior.
> > >>
> > >> thoughts ?
> > >> Harmeet
> > >
> > >
> > >
> > >
> >
> >
Re: Developers Note: Screen Widget Model Classes
Posted by Adrian Crum <ad...@yahoo.com>.
That could be done too. I don't see where it would be safer. You can push/pop a state object in the renderer.
-Adrian
--- On Fri, 5/28/10, Harmeet Bedi <ha...@gmail.com> wrote:
> > The state data doesn't need to
> be stored in the model objects ... can
> be stored just as easily in the renderer.
>
> I think safer practice should be to store state in context
> not renderer.
> So push new map on context, call sub-widget rendering and
> pop map on
> child widget render finish.
>
> Renderer instance is single per screen render. I think of
> it as
> something like global context. So would need to clean state
> after use.
>
> Context(MapStack) may be better suited for this ?
>
> Harmeet
>
> On 29/05/10 4:23 AM, Adrian Crum wrote:
> > --- On Fri, 5/28/10, Harmeet Bedi<ha...@gmail.com>
> wrote:
> >> Forms/Screens are specified in XML with a key -
> based on
> >> xml file and name of element. XML Specifications
> is
> >> converted into Object model using model classes.
> These
> >> objects are then used to render.
> >>
> >> Since model classes are cached and shared, the
> model must
> >> accurately represent the XML specification at the
> start of
> >> rendering.
> >>
> >> So a way could be immutable classes
> >> Another way could be to return cloned copy of
> cached object
> >> at the start of rendering. If you do this it does
> not matter
> >> if anyone makes a mistake in the dozens of model
> classes
> >> wrt. cloned. (which many people may do over life
> of project
> >> because it is human to make stupid mistakes)
> >>
> >> It may also be worth asking why cache. To me
> answer is
> >> performance due to cost of XML parsing and
> conversion from
> >> XML to object model.
> >> Creating Flexible string expander instance
> variables in the
> >> model classes may also be a cost but it is not due
> to
> >> FlexibleStringExpander cache.
> >
> > Actually, there is no reason to create
> FlexibleStringExpander instances - you can call the static
> expandString method with very little performance loss.
> >
> >> Wondering if the following proposal makes sense:
> >> - Generate Model Classes from XSD using something
> like
> >> JAXB. These would not have any flexible string
> expander
> >> instance variables.
> >> - Cache the key (resource+name) to Model for
> lookup of
> >> screens and forms
> >> - In cache lookup. Clone copy of Model root. Wrap
> the
> >> cloned copy with a dynamic proxy. Dynamic proxy
> also stores
> >> reference to context.
> >> - Getters from the dynamic proxy return values by
> applying
> >> FlexibleStringExpander.getInstance(..) on the
> value obtained
> >> from underlying cloned model.
> >> - Change Render classes to use these generated
> JAXB
> >> objects.
> >
> > That would eliminate the unmarshalling code.
> >
> >> Advantage could be
> >> - Most of the handwritten code for Model Classes
> goes
> >> away.
> >
> > I don't know about "most." From my perspective, the
> unmarshalling code is a small percentage of the screen
> widget code (basically just the class constructors).
> >
> > If we used the builder pattern, we could unmarshall
> the XML files by using a builder class that builds the model
> objects. Builder classes could build model objects from XML
> files, or from another source - like a database, a CMS, or
> another file format.
> >
> >> - Model classes and XML specification are tied
> together and
> >> specified only by XSD.
> >> - Rendering presents a pipeline. Model classes can
> be
> >> modified by renderer if needed as the pipeline
> proceeds from
> >> start of rendering till end.
> >
> > The state data doesn't need to be stored in the model
> objects - it's only stored there now because the developer
> who put it there didn't know any better. It can be stored
> just as easily in the renderer. There is no need to clone
> model objects.
> >
> >> - Dynamic proxies are small. Mostly standard
> conversion
> >> (apply FlexibleStringExpander::expand) but it
> could be the
> >> spot to add any additional behavior.
> >>
> >> thoughts ?
> >> Harmeet
> >
> >
> >
> >
>
>
Re: Developers Note: Screen Widget Model Classes
Posted by Harmeet Bedi <ha...@gmail.com>.
> The state data doesn't need to be stored in the model objects ... can
be stored just as easily in the renderer.
I think safer practice should be to store state in context not renderer.
So push new map on context, call sub-widget rendering and pop map on
child widget render finish.
Renderer instance is single per screen render. I think of it as
something like global context. So would need to clean state after use.
Context(MapStack) may be better suited for this ?
Harmeet
On 29/05/10 4:23 AM, Adrian Crum wrote:
> --- On Fri, 5/28/10, Harmeet Bedi<ha...@gmail.com> wrote:
>> Forms/Screens are specified in XML with a key - based on
>> xml file and name of element. XML Specifications is
>> converted into Object model using model classes. These
>> objects are then used to render.
>>
>> Since model classes are cached and shared, the model must
>> accurately represent the XML specification at the start of
>> rendering.
>>
>> So a way could be immutable classes
>> Another way could be to return cloned copy of cached object
>> at the start of rendering. If you do this it does not matter
>> if anyone makes a mistake in the dozens of model classes
>> wrt. cloned. (which many people may do over life of project
>> because it is human to make stupid mistakes)
>>
>> It may also be worth asking why cache. To me answer is
>> performance due to cost of XML parsing and conversion from
>> XML to object model.
>> Creating Flexible string expander instance variables in the
>> model classes may also be a cost but it is not due to
>> FlexibleStringExpander cache.
>
> Actually, there is no reason to create FlexibleStringExpander instances - you can call the static expandString method with very little performance loss.
>
>> Wondering if the following proposal makes sense:
>> - Generate Model Classes from XSD using something like
>> JAXB. These would not have any flexible string expander
>> instance variables.
>> - Cache the key (resource+name) to Model for lookup of
>> screens and forms
>> - In cache lookup. Clone copy of Model root. Wrap the
>> cloned copy with a dynamic proxy. Dynamic proxy also stores
>> reference to context.
>> - Getters from the dynamic proxy return values by applying
>> FlexibleStringExpander.getInstance(..) on the value obtained
>> from underlying cloned model.
>> - Change Render classes to use these generated JAXB
>> objects.
>
> That would eliminate the unmarshalling code.
>
>> Advantage could be
>> - Most of the handwritten code for Model Classes goes
>> away.
>
> I don't know about "most." From my perspective, the unmarshalling code is a small percentage of the screen widget code (basically just the class constructors).
>
> If we used the builder pattern, we could unmarshall the XML files by using a builder class that builds the model objects. Builder classes could build model objects from XML files, or from another source - like a database, a CMS, or another file format.
>
>> - Model classes and XML specification are tied together and
>> specified only by XSD.
>> - Rendering presents a pipeline. Model classes can be
>> modified by renderer if needed as the pipeline proceeds from
>> start of rendering till end.
>
> The state data doesn't need to be stored in the model objects - it's only stored there now because the developer who put it there didn't know any better. It can be stored just as easily in the renderer. There is no need to clone model objects.
>
>> - Dynamic proxies are small. Mostly standard conversion
>> (apply FlexibleStringExpander::expand) but it could be the
>> spot to add any additional behavior.
>>
>> thoughts ?
>> Harmeet
>
>
>
>
Re: Developers Note: Screen Widget Model Classes
Posted by Adrian Crum <ad...@yahoo.com>.
--- On Fri, 5/28/10, Harmeet Bedi <ha...@gmail.com> wrote:
> Forms/Screens are specified in XML with a key - based on
> xml file and name of element. XML Specifications is
> converted into Object model using model classes. These
> objects are then used to render.
>
> Since model classes are cached and shared, the model must
> accurately represent the XML specification at the start of
> rendering.
>
> So a way could be immutable classes
> Another way could be to return cloned copy of cached object
> at the start of rendering. If you do this it does not matter
> if anyone makes a mistake in the dozens of model classes
> wrt. cloned. (which many people may do over life of project
> because it is human to make stupid mistakes)
>
> It may also be worth asking why cache. To me answer is
> performance due to cost of XML parsing and conversion from
> XML to object model.
> Creating Flexible string expander instance variables in the
> model classes may also be a cost but it is not due to
> FlexibleStringExpander cache.
Actually, there is no reason to create FlexibleStringExpander instances - you can call the static expandString method with very little performance loss.
> Wondering if the following proposal makes sense:
> - Generate Model Classes from XSD using something like
> JAXB. These would not have any flexible string expander
> instance variables.
> - Cache the key (resource+name) to Model for lookup of
> screens and forms
> - In cache lookup. Clone copy of Model root. Wrap the
> cloned copy with a dynamic proxy. Dynamic proxy also stores
> reference to context.
> - Getters from the dynamic proxy return values by applying
> FlexibleStringExpander.getInstance(..) on the value obtained
> from underlying cloned model.
> - Change Render classes to use these generated JAXB
> objects.
That would eliminate the unmarshalling code.
> Advantage could be
> - Most of the handwritten code for Model Classes goes
> away.
I don't know about "most." From my perspective, the unmarshalling code is a small percentage of the screen widget code (basically just the class constructors).
If we used the builder pattern, we could unmarshall the XML files by using a builder class that builds the model objects. Builder classes could build model objects from XML files, or from another source - like a database, a CMS, or another file format.
> - Model classes and XML specification are tied together and
> specified only by XSD.
> - Rendering presents a pipeline. Model classes can be
> modified by renderer if needed as the pipeline proceeds from
> start of rendering till end.
The state data doesn't need to be stored in the model objects - it's only stored there now because the developer who put it there didn't know any better. It can be stored just as easily in the renderer. There is no need to clone model objects.
> - Dynamic proxies are small. Mostly standard conversion
> (apply FlexibleStringExpander::expand) but it could be the
> spot to add any additional behavior.
>
> thoughts ?
> Harmeet
Re: Developers Note: Screen Widget Model Classes
Posted by Harmeet Bedi <ha...@gmail.com>.
Here is how i think system works.
Forms/Screens are specified in XML with a key - based on xml file and
name of element. XML Specifications is converted into Object model using
model classes. These objects are then used to render.
Since model classes are cached and shared, the model must accurately
represent the XML specification at the start of rendering.
So a way could be immutable classes
Another way could be to return cloned copy of cached object at the start
of rendering. If you do this it does not matter if anyone makes a
mistake in the dozens of model classes wrt. cloned. (which many people
may do over life of project because it is human to make stupid mistakes)
It may also be worth asking why cache. To me answer is performance due
to cost of XML parsing and conversion from XML to object model.
Creating Flexible string expander instance variables in the model
classes may also be a cost but it is not due to FlexibleStringExpander
cache.
Wondering if the following proposal makes sense:
- Generate Model Classes from XSD using something like JAXB. These would
not have any flexible string expander instance variables.
- Cache the key (resource+name) to Model for lookup of screens and forms
- In cache lookup. Clone copy of Model root. Wrap the cloned copy with a
dynamic proxy. Dynamic proxy also stores reference to context.
- Getters from the dynamic proxy return values by applying
FlexibleStringExpander.getInstance(..) on the value obtained from
underlying cloned model.
- Change Render classes to use these generated JAXB objects.
Advantage could be
- Most of the handwritten code for Model Classes goes away.
- Model classes and XML specification are tied together and specified
only by XSD.
- Rendering presents a pipeline. Model classes can be modified by
renderer if needed as the pipeline proceeds from start of rendering till
end.
- Dynamic proxies are small. Mostly standard conversion (apply
FlexibleStringExpander::expand) but it could be the spot to add any
additional behavior.
thoughts ?
Harmeet
Re: Developers Note: Screen Widget Model Classes
Posted by Adam Heath <do...@brainfood.com>.
Jacques Le Roux wrote:
> From: "Adam Heath" <do...@brainfood.com>
>> Adrian Crum wrote:
>>>>> Because there is a difference of opinion about widget model
>>>>> immutability. I had suggested those changes a few years ago and there
>>>>> was opposition to it.
>>>>
>>>> What was the opposition?
>>>>
>>>> 1: it's stupid
>>>> 2: a conversation like this:
>>>> person-a: I think the model should be immutable. You should do it.
>>>> person-b: I won't do it.
>>>> person-a: But it's the best thing since sliced bread!
>>>> person-b: You can't force me to do anything.
>>>> 3: making the model immutable would remove locking, yes, but then the
>>>> renderer classes would get more complex, and harder to
>>>> write/understand. The cost-benefit ratio just doesn't justify it.
>>>
>>> 4: We don't want to remove flexibility.
>>
>> flexibility here meaning the ability to write broken code?
>
>
> Listen guys, some fun ahead :D
I've got some mud.
Re: Developers Note: Screen Widget Model Classes
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Adam Heath" <do...@brainfood.com>
> Adrian Crum wrote:
>>>> Because there is a difference of opinion about widget model
>>>> immutability. I had suggested those changes a few years ago and there
>>>> was opposition to it.
>>>
>>> What was the opposition?
>>>
>>> 1: it's stupid
>>> 2: a conversation like this:
>>> person-a: I think the model should be immutable. You should do it.
>>> person-b: I won't do it.
>>> person-a: But it's the best thing since sliced bread!
>>> person-b: You can't force me to do anything.
>>> 3: making the model immutable would remove locking, yes, but then the
>>> renderer classes would get more complex, and harder to
>>> write/understand. The cost-benefit ratio just doesn't justify it.
>>
>> 4: We don't want to remove flexibility.
>
> flexibility here meaning the ability to write broken code?
Listen guys, some fun ahead :D
Jacques
Re: Developers Note: Screen Widget Model Classes
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
>>> Because there is a difference of opinion about widget model
>>> immutability. I had suggested those changes a few years ago and there
>>> was opposition to it.
>>
>> What was the opposition?
>>
>> 1: it's stupid
>> 2: a conversation like this:
>> person-a: I think the model should be immutable. You should do it.
>> person-b: I won't do it.
>> person-a: But it's the best thing since sliced bread!
>> person-b: You can't force me to do anything.
>> 3: making the model immutable would remove locking, yes, but then the
>> renderer classes would get more complex, and harder to
>> write/understand. The cost-benefit ratio just doesn't justify it.
>
> 4: We don't want to remove flexibility.
flexibility here meaning the ability to write broken code?
Re: Developers Note: Screen Widget Model Classes
Posted by Adrian Crum <ad...@hlmksw.com>.
On 5/26/2010 8:25 AM, Adam Heath wrote:
> Adrian Crum wrote:
>> On 5/26/2010 7:01 AM, Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> Just a friendly reminder to the OFBiz developers and committers: The
>>>> screen widget model classes should not have their state altered by
>>>> renderers. That is not thread-safe and it results in undesirable
>>>> behavior.
>>>>
>>>> I know it's easy to forget - I just deprecated some methods that I wrote
>>>> where I made that mistake. :-)
>>>>
>>>> It might help to think of the widget model classes as being immutable
>>>> (though technically they aren't). Once they are constructed and put in
>>>> the cache, they shouldn't be changed.
>>>
>>> Then why not be explicit about it? Make the classes final(maybe),
>>> make their fields final(required), set all such fields only in the
>>> constructor(required). If construction is more dynamic/complex, use a
>>> Builder pattern.
>>
>> Because there is a difference of opinion about widget model
>> immutability. I had suggested those changes a few years ago and there
>> was opposition to it.
>
> What was the opposition?
>
> 1: it's stupid
> 2: a conversation like this:
> person-a: I think the model should be immutable. You should do it.
> person-b: I won't do it.
> person-a: But it's the best thing since sliced bread!
> person-b: You can't force me to do anything.
> 3: making the model immutable would remove locking, yes, but then the
> renderer classes would get more complex, and harder to
> write/understand. The cost-benefit ratio just doesn't justify it.
4: We don't want to remove flexibility.
> Having classes immutable means no locking needs to be done to access
> their data. Immutable in this case means that if they contain
> references to complex objects, then those objects must also be
> immutable(collections classes, etc).
>
> Also, when immutable classes are used heavily, it tends to lead to
> algorithms that end up creating local-to-the-method variables to
> maintain their work state. java1.6 made that *very* fast, by being
> able to detect this(escape analysis), and such objects no longer get
> allocated on the heap, but on the stack(which reduces contention, and
> reduces GC overhead).
Agreed. My main point at the time was to enforce immutability so that
developers can't make the mistake of writing code that is not thread-safe.
I recently created some Jira issues related to this subject, and some of
the thread-unsafe code they report on was committed since I had that
initial conversation. As long as the model widgets are kept mutable, we
will continue to repeat this error.
-Adrian
Re: Developers Note: Screen Widget Model Classes
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> On 5/26/2010 7:01 AM, Adam Heath wrote:
>> Adrian Crum wrote:
>>> Just a friendly reminder to the OFBiz developers and committers: The
>>> screen widget model classes should not have their state altered by
>>> renderers. That is not thread-safe and it results in undesirable
>>> behavior.
>>>
>>> I know it's easy to forget - I just deprecated some methods that I wrote
>>> where I made that mistake. :-)
>>>
>>> It might help to think of the widget model classes as being immutable
>>> (though technically they aren't). Once they are constructed and put in
>>> the cache, they shouldn't be changed.
>>
>> Then why not be explicit about it? Make the classes final(maybe),
>> make their fields final(required), set all such fields only in the
>> constructor(required). If construction is more dynamic/complex, use a
>> Builder pattern.
>
> Because there is a difference of opinion about widget model
> immutability. I had suggested those changes a few years ago and there
> was opposition to it.
What was the opposition?
1: it's stupid
2: a conversation like this:
person-a: I think the model should be immutable. You should do it.
person-b: I won't do it.
person-a: But it's the best thing since sliced bread!
person-b: You can't force me to do anything.
3: making the model immutable would remove locking, yes, but then the
renderer classes would get more complex, and harder to
write/understand. The cost-benefit ratio just doesn't justify it.
Having classes immutable means no locking needs to be done to access
their data. Immutable in this case means that if they contain
references to complex objects, then those objects must also be
immutable(collections classes, etc).
Also, when immutable classes are used heavily, it tends to lead to
algorithms that end up creating local-to-the-method variables to
maintain their work state. java1.6 made that *very* fast, by being
able to detect this(escape analysis), and such objects no longer get
allocated on the heap, but on the stack(which reduces contention, and
reduces GC overhead).
Re: Developers Note: Screen Widget Model Classes
Posted by Adrian Crum <ad...@hlmksw.com>.
On 5/26/2010 7:01 AM, Adam Heath wrote:
> Adrian Crum wrote:
>> Just a friendly reminder to the OFBiz developers and committers: The
>> screen widget model classes should not have their state altered by
>> renderers. That is not thread-safe and it results in undesirable behavior.
>>
>> I know it's easy to forget - I just deprecated some methods that I wrote
>> where I made that mistake. :-)
>>
>> It might help to think of the widget model classes as being immutable
>> (though technically they aren't). Once they are constructed and put in
>> the cache, they shouldn't be changed.
>
> Then why not be explicit about it? Make the classes final(maybe),
> make their fields final(required), set all such fields only in the
> constructor(required). If construction is more dynamic/complex, use a
> Builder pattern.
Because there is a difference of opinion about widget model
immutability. I had suggested those changes a few years ago and there
was opposition to it.
-Adrian
Re: Developers Note: Screen Widget Model Classes
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Just a friendly reminder to the OFBiz developers and committers: The
> screen widget model classes should not have their state altered by
> renderers. That is not thread-safe and it results in undesirable behavior.
>
> I know it's easy to forget - I just deprecated some methods that I wrote
> where I made that mistake. :-)
>
> It might help to think of the widget model classes as being immutable
> (though technically they aren't). Once they are constructed and put in
> the cache, they shouldn't be changed.
Then why not be explicit about it? Make the classes final(maybe),
make their fields final(required), set all such fields only in the
constructor(required). If construction is more dynamic/complex, use a
Builder pattern.
immutable classes don't require locking, which is also a good goal to
strive for.
Re: Developers Note: Screen Widget Model Classes
Posted by Adrian Crum <ad...@yahoo.com>.
Fixing the thread safety issue is not too hard - it just involves moving the fields that change state from the model classes to the rendering classes.
I had suggested removing model setXxx methods a while ago and there was some disagreement about that.
-Adrian
--- On Wed, 5/26/10, Harmeet Bedi <ha...@gmail.com> wrote:
> From: Harmeet Bedi <ha...@gmail.com>
> Subject: Re: Developers Note: Screen Widget Model Classes
> To: dev@ofbiz.apache.org
> Date: Wednesday, May 26, 2010, 2:11 AM
> Wondering if these 2 patterns could
> make sense:
>
> - Pass clone of ModelFormField instance to renderer and
> have each instanceof ModelFormField implement Cloneable
> contract. Could provide a systematic way to improve
> thread safety
> Wondering if that would be too expensive. 2 things that may
> help:
> FlexibleStringExpander instances or any other immutable
> references could be shared between cloned and 'this'.
> FlexibleStringExpander can be assumed immutable.
> Incremental gc in java could mitigate overhead.
>
> - Restrict access or remove setXXX methods in
> ModelFormField instances.
>
> Had similar problems a while back when i extended
> renderers/xsd in our code. So feel the pain. Workaround was
> to use only context reference to build pipeline if state
> needs to be carried and not change the object.
>
> Harmeet
>
> On 19/05/10 1:49 PM, Adrian Crum wrote:
> > Just a friendly reminder to the OFBiz developers and
> committers: The
> > screen widget model classes should not have their
> state altered by
> > renderers. That is not thread-safe and it results in
> undesirable behavior.
> >
> > I know it's easy to forget - I just deprecated some
> methods that I wrote
> > where I made that mistake. :-)
> >
> > It might help to think of the widget model classes as
> being immutable
> > (though technically they aren't). Once they are
> constructed and put in
> > the cache, they shouldn't be changed.
> >
> > If you see code where a renderer calls a model's
> renderXxxx method, and
> > inside that method the model's fields are being
> changed, then that is a
> > no-no.
> >
> > -Adrian
> >
>
>
Re: Developers Note: Screen Widget Model Classes
Posted by Harmeet Bedi <ha...@gmail.com>.
Wondering if these 2 patterns could make sense:
- Pass clone of ModelFormField instance to renderer and have each
instanceof ModelFormField implement Cloneable contract. Could provide a
systematic way to improve thread safety
Wondering if that would be too expensive. 2 things that may help:
FlexibleStringExpander instances or any other immutable references could
be shared between cloned and 'this'. FlexibleStringExpander can be
assumed immutable.
Incremental gc in java could mitigate overhead.
- Restrict access or remove setXXX methods in ModelFormField instances.
Had similar problems a while back when i extended renderers/xsd in our
code. So feel the pain. Workaround was to use only context reference to
build pipeline if state needs to be carried and not change the object.
Harmeet
On 19/05/10 1:49 PM, Adrian Crum wrote:
> Just a friendly reminder to the OFBiz developers and committers: The
> screen widget model classes should not have their state altered by
> renderers. That is not thread-safe and it results in undesirable behavior.
>
> I know it's easy to forget - I just deprecated some methods that I wrote
> where I made that mistake. :-)
>
> It might help to think of the widget model classes as being immutable
> (though technically they aren't). Once they are constructed and put in
> the cache, they shouldn't be changed.
>
> If you see code where a renderer calls a model's renderXxxx method, and
> inside that method the model's fields are being changed, then that is a
> no-no.
>
> -Adrian
>