Hi Bart, I've attached the patch to SiteMenuLinkRewriterImpl. We still have to do a extensive test (hopefully next weekend) to determine how well this patch works. I'll let you know about the results.
Regards,
Wouter Zelle
On Fri, Mar 12, 2010 at 11:09, Bart van der Schans <
b.vandersch...@onehippo.com> wrote:
> Hi Wouter,
>
> Thanks for your analysis. I'm not that familiar with the
> SitemenuLinkRewriterTranformer and it has been a long time since I had
> a look at the code. It sounds like there is something going wrong in
> there. I agree with your analysis and proposed (line of) change. Could
> you create an initial patch?
>
> Regards,
> Bart
>
> On Thu, Mar 11, 2010 at 12:40 PM, Wouter Zelle <wouter.ze...@gmail.com>
> wrote:
> > Hi,
> >
> > Occasionally we see incorrect translations of documents in the repository
> to
> > external urls. The same SitemenuLinkRewriterTranformer should never be
> used
> > by seperate threads, so the cause is probably in the
> > SiteMenuLinkRewriterImpl. One of the incorrect translations is for a
> > 'perfect match' (location is the same as the datasource), so the error
> > should be between lines 164-200 of the SiteMenuLinkRewriterImpl. In that
> > code range I noticed a fairly classic multi-threading bug (roughly
> speaking,
> > it is 'double-checked locking') on line 168:
> >
> > if (!(cache.containsKey(cacheKeyList) && cache.containsKey(cacheKeyMap)))
> {
> > initialize(host);
> > }
> >
> > The initialize method is synchronized, but since the check isn't, a later
> > thread can use a semi-initialized cache, while an earlier thread is still
> in
> > the initialize method. A partial solution to this problem might be to
> place
> > a synchronize-block (locking the cache) around the code above (removing
> the
> > synchronization from the initialize method), which should prevent this
> > problem, would prevent duplicate initializations and would block other
> > threads (who are already translating) from seeing an partially
> initialized
> > cache (since the cache has synchronized methods, so cache.get will block
> > during the initialize). Of course, threads still might have to deal with
> a
> > cache that empties and/or get filled half-way through a translate, but
> > solving this requires a lot more synchronization.
> >
> > I expect that the performance penalty of this change will be minimal. Do
> you
> > agree that this might be a good change?
> >
> > Regards,
> >
> > Wouter Zelle
> > ********************************************
> > Hippocms-dev: Hippo CMS 6 development public mailinglist
> >
> > Searchable archives can be found at:
> > MarkMail: http://hippocms-dev.markmail.org
> > Nabble: http://www.nabble.com/Hippo-CMS-f26633.html
> >
> >
>
>
>
> --
> Hippo B.V. - Amsterdam
> Oosteinde 11, 1017 WT, Amsterdam, +31(0)20-5224466
>
> Hippo USA Inc. - San Francisco
> 101 H Street, Suite Q, Petaluma CA, 94952-3329, +1 (707) 773-4646
> -----------------------------------------------------------------
> http://www.onehippo.com - i...@onehippo.com
> -----------------------------------------------------------------
> ********************************************
> Hippocms-dev: Hippo CMS 6 development public mailinglist
>
> Searchable archives can be found at:
> MarkMail: http://hippocms-dev.markmail.org
> Nabble: http://www.nabble.com/Hippo-CMS-f26633.html
>
>
threadsafety.patch
Description: Binary data
******************************************** Hippocms-dev: Hippo CMS 6 development public mailinglist Searchable archives can be found at: MarkMail: http://hippocms-dev.markmail.org Nabble: http://www.nabble.com/Hippo-CMS-f26633.html
