Re: fop-0.92.beta: Thread safety issues with FOP instance construction

[Jeremias Maerki <de...@jeremias-maerki.ch>]

I'm sure everyone has seen this thread on fop-users. Any particular
preferences on one of the two options I listed below? The first will
require coordination with Batik as they are supposed to migrate to that
class and it's basically their version. We used a modified version of
theirs. The second is probably cleaner but since the change is
backwards-incompatible for any ElementMapping implementation (Barcode4J,
for example), this is not to be taken lightly. I think the first is
easier, should not have any side-effects but feels more like patchwork.

On 15.05.2006 09:27:29 Jeremias Maerki wrote:
> 
> On 13.05.2006 00:45:51 Ryan Gustafson wrote:
> > Hi Jeremias,
> > 
> > > The Service class is properly synchronized.
> > 
> > I downloaded the source, and agree.
> > 
> > > The ElementMapping
> > > classes, however, are recreated for each FopFactory. There's no
> > > sharing of these classes between multiple FopFactory instances.
> > 
> > I downloaded the XML Graphics Common code.  And I've reviewed the
> > FOP code.  I do not see it this way, see my further comments
> > below...
> > 
> > > Since you're using the deprecated Fop constructor a new
> > > FopFactory is instantiated for each new Fop instance. So I don't
> > > see why the ElementMapping classes need to be thread-safe.
> > 
> > It's true, we have unique FOP instances, each with unique FopFactory
> > instances, each with a unique ElementMappingRegistry instance. 
> > Where they all come together is in the ElementMappingRegistry static
> > call to Service.providers(), which based on the code I saw in the
> > XML Graphics Common 1.0 version of  Service.java, would appear to
> > return an Iterator over the _same_ statically cached list List,
> > which then contains the same ElementMapping objects.  Thus, the
> > FopFactory objects end up using the same ElementMapping objects.
> > 
> > The Service code appears to be using standard ArrayList and HashMap,
> > so the Map.put()/Map.get() and List.iterator() method work normally
> > (no behind the scenes cloning/copying going on).  Further, it is
> > directly placing a normal FOElementMapping instance into the list
> > (no special wrapping going on).
> 
> Right, I should have placed a breakpoint earlier to see what happens. I
> was under the assumption that only the class names are stored in the
> list. The reason for that: We used a modified version of Batik's Service
> class which only saved the class names. When I populated XML Graphics
> Commons I took Batik's implementation and adjusted FOP's code to use
> that. Turns out this wasn't without side-effects. From this POV, the
> multi-threading issue is absolutely logical.
> 
> > Should the Service be returning caller unique instances in the
> > Iterator?  If so, it needs to be re-written.  The JavaDoc doesn't
> > give me the feeling that is supposed to do this however.
> 
> I see two possible routes:
> 1. Adding an optional parameter to the Service class' providers() method
> which allows to return class names instead of instances. This will
> restore the previous behaviour and maintain backwards compatibility with
> the existing ElementMapping implementations.
> 2. Change ElementMapping's constructor to accept the namespace URI as a
> parameter and call initialize() right after that. Drawback: It's a
> backwards-incompatible change.
> 
> We'll discuss that on fop-dev and decide what to do.
> 
> > I only spent about 20 minutes looking over this, so I may have
> > overlooked something?
> > 
> > > I don't buy it, yet. I can see from your stack trace that
> > something's
> > > wrong but I'm not sure you've found the right spot. What you can
> > > try is to synchronize ElementMapping.getTable() and check if this
> > > changes anything, but I just don't understand why two threads
> > > would modify the same HashMap in ElementMapping.
> > >
> > > I wish I could reproduce the problem. I've tried yesterday, but
> > > didn't succeed. I ran up to 60 threads in parallel with the
> > > deprecated constructor and had no problems. But on the other
> > > hand, I only had a single-processor machine to test on. Are you
> > > on a multi-processor box
> > 
> > Yup, multi-processor box.  That definately makes it easier to expose
> > thread safety issues.  I believe we were running about 5 thread max
> > through the FOP code.  Also we've only seen it once so far, it may
> > be tricky to trigger.
> > 
> > Tell you what, I'm due to take over working on our FOP related code
> > in a couple weeks.  When I do, I'll keep my eye out for this issue,
> > and if I find out anything more, I'll do what I can to find the
> > source and share what I find.
> > 
> > In the meantime, we'll move to the non-deprecated API, and presume
> > the issue is solved, until we get more Exceptions to prove
> > otherwise.
> 
> Certainly a good move. I believe I know now what's wrong so we can take
> appropriate action. Thanks for being so persistent.



Jeremias Maerki

Re: fop-0.92.beta: Thread safety issues with FOP instance construction

[Chris Bowditch <bo...@hotmail.com>]

Jeremias Maerki wrote:

> I'm sure everyone has seen this thread on fop-users. Any particular
> preferences on one of the two options I listed below? The first will
> require coordination with Batik as they are supposed to migrate to that
> class and it's basically their version. We used a modified version of
> theirs. The second is probably cleaner but since the change is
> backwards-incompatible for any ElementMapping implementation (Barcode4J,
> for example), this is not to be taken lightly. I think the first is
> easier, should not have any side-effects but feels more like patchwork.

I prefer the first approach. I don't like the idea of backwards 
incompatible changes for Extension classes.

Chris

Re: fop-0.92.beta: Thread safety issues with FOP instance construction

[Simon Pepping <sp...@leverkruid.eu>]

On Mon, May 15, 2006 at 09:33:49AM +0200, Jeremias Maerki wrote:
> I'm sure everyone has seen this thread on fop-users. Any particular
> preferences on one of the two options I listed below? The first will
> require coordination with Batik as they are supposed to migrate to that
> class and it's basically their version. We used a modified version of
> theirs. The second is probably cleaner but since the change is
> backwards-incompatible for any ElementMapping implementation (Barcode4J,
> for example), this is not to be taken lightly. I think the first is
> easier, should not have any side-effects but feels more like patchwork.

The first option sounds fine with me.

> > I see two possible routes:
> > 1. Adding an optional parameter to the Service class' providers() method
> > which allows to return class names instead of instances. This will
> > restore the previous behaviour and maintain backwards compatibility with
> > the existing ElementMapping implementations.
> > 2. Change ElementMapping's constructor to accept the namespace URI as a
> > parameter and call initialize() right after that. Drawback: It's a
> > backwards-incompatible change.

Simon

-- 
Simon Pepping
home page: http://www.leverkruid.eu