You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adam Heath <do...@brainfood.com> on 2010/02/14 01:52:28 UTC
AbstractConverter not thread safe
Object creation can't be synchronized. As such, any variable
assignments done in any chained constructors can be reordered at will
by the VM. The only guarantee you have is that when the new call is
finished, the object is constructed.
AbstractConverter breaks this. In it's constructor, it registers
'this' with an external entity(Converters.converterMap). This leaves
a window where another thread could request the new converter to do
some work, when the object is not yet finished getting constructed.
This is a big nono.
No constructor *anywhere* can add this to *any* external system. It
*must* take place after the object has finished constructing. This
can be done either with a method on the object itself, or completely
external to the class hierarchy.
The fix in this case, is first to change
Converts.loadContainedConverters, to test whether the new object
implements ConverterLoader, and if so, call it's loadConverters
method. Then, make AbstractConverter implement ConverterLoader, and
move it's register tall to loadConverters. This will also give
sub-classes a convenient way to register themselves for other class pairs.
Re: AbstractConverter not thread safe
Posted by Adrian Crum <ad...@yahoo.com>.
--- On Sat, 2/13/10, Adam Heath <do...@brainfood.com> wrote:
> From: Adam Heath <do...@brainfood.com>
> Subject: Re: AbstractConverter not thread safe
> To: dev@ofbiz.apache.org
> Date: Saturday, February 13, 2010, 4:56 PM
> Adam Heath wrote:
> > Object creation can't be synchronized. As such,
> any variable
> > assignments done in any chained constructors can be
> reordered at will
> > by the VM. The only guarantee you have is that
> when the new call is
> > finished, the object is constructed.
> >
> > AbstractConverter breaks this. In it's
> constructor, it registers
> > 'this' with an external
> entity(Converters.converterMap). This leaves
> > a window where another thread could request the new
> converter to do
> > some work, when the object is not yet finished getting
> constructed.
> > This is a big nono.
> >
> > No constructor *anywhere* can add this to *any*
> external system. It
> > *must* take place after the object has finished
> constructing. This
> > can be done either with a method on the object itself,
> or completely
> > external to the class hierarchy.
> >
> > The fix in this case, is first to change
> > Converts.loadContainedConverters, to test whether the
> new object
> > implements ConverterLoader, and if so, call it's
> loadConverters
> > method. Then, make AbstractConverter implement
> ConverterLoader, and
> > move it's register tall to loadConverters. This
> will also give
> > sub-classes a convenient way to register themselves
> for other class pairs.
>
> ps: This is straight out of the Java Concurrency
> Programming in
> Practice book. It actually ranks above the data model
> books, afaiac.
I ordered it at the beginning of this week, it should be arriving soon.
Re: AbstractConverter not thread safe
Posted by Adam Heath <do...@brainfood.com>.
Adam Heath wrote:
> Object creation can't be synchronized. As such, any variable
> assignments done in any chained constructors can be reordered at will
> by the VM. The only guarantee you have is that when the new call is
> finished, the object is constructed.
>
> AbstractConverter breaks this. In it's constructor, it registers
> 'this' with an external entity(Converters.converterMap). This leaves
> a window where another thread could request the new converter to do
> some work, when the object is not yet finished getting constructed.
> This is a big nono.
>
> No constructor *anywhere* can add this to *any* external system. It
> *must* take place after the object has finished constructing. This
> can be done either with a method on the object itself, or completely
> external to the class hierarchy.
>
> The fix in this case, is first to change
> Converts.loadContainedConverters, to test whether the new object
> implements ConverterLoader, and if so, call it's loadConverters
> method. Then, make AbstractConverter implement ConverterLoader, and
> move it's register tall to loadConverters. This will also give
> sub-classes a convenient way to register themselves for other class pairs.
ps: This is straight out of the Java Concurrency Programming in
Practice book. It actually ranks above the data model books, afaiac.