cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vadim Gritsenko <va...@reverycodes.com>
Subject Re: svn commit: r156143 - cocoon/trunk/src/java/org/apache/cocoon/generation/JXTemplateGenerator.java cocoon/trunk/status.xml
Date Wed, 09 Mar 2005 13:07:27 GMT
Leszek Gawron wrote:
> Vadim Gritsenko wrote:
> 
>> lgawron@apache.org wrote:
>>
>>> Author: lgawron
>>> Date: Fri Mar  4 00:39:53 2005
>>> New Revision: 156143
>>>
>>> URL: http://svn.apache.org/viewcvs?view=rev&rev=156143
>>> Log:
>>> Fix thread safety problem in JXTemplateGenerator.setup() concerning 
>>> template script reparsing.
>>>
>>> Modified: 
>>> cocoon/trunk/src/java/org/apache/cocoon/generation/JXTemplateGenerator.java 
>>>
>>> URL: 
>>> http://svn.apache.org/viewcvs/cocoon/trunk/src/java/org/apache/cocoon/generation/JXTemplateGenerator.java?view=diff&r1=156142&r2=156143

>>>
>>> ==============================================================================

>>>
>>> --- 
>>> cocoon/trunk/src/java/org/apache/cocoon/generation/JXTemplateGenerator.java 
>>> (original)
>>> +++ 
>>> cocoon/trunk/src/java/org/apache/cocoon/generation/JXTemplateGenerator.java 
>>> Fri Mar  4 00:39:53 2005
>>> @@ -2357,7 +2357,6 @@
>>>                          valid = 
>>> startEvent.compileTime.isValid(validity);
>>>                      }
>>>                      if (valid != SourceValidity.VALID) {
>>> -                        cache.remove(uri);
>>>                          regenerate = true;
>>>                      }
>>>                  } else {
>>
>>
>>
>> What good this does? Second thread, instead of quick fail on first 
>> test (if (startEvent != null)), will go through complete validity 
>> check. And it will arrive to the same result, that it need to 
>> re-generate from source. So, it will arrive to the same 
>> SourceUtil.parse line.
>>
>> PS I'd mark the bug INVALID as I do not see what's broken. Yes, it can 
>> parse it twice. It's not an error by itself. Wrapping whole parsing 
>> block into the synchronized() is abviously not a solution too, 
>> especially for larger sites.
> 
> Maybe I had it wrong but imagine the situation with two threads:
>       Thread1                      Thread2
> 1) JXTG.setup()
>    got script,
>    script is valid
>    JXTG.setup() finish
> 
> 2)                                JXTG.setup()
>                                   script invalid so removed from cache
> 
> 3) JXTG.generate()
>    script = cache.get(...)
>    script is null as second
>    thread removed it while
>    it should still be there
>    because it was valid at the
>    setup() stage
> 
> 4)                                script reparsed
>                                   JXTG.setup() finish()
> 
> 
> This is not a race condition between two JXTG.setup() but with 
> JXTG.generate() in first thread and JXTG.setup() in second one.

Ok, got it. In this case proper solution is not to use cache in the generate, 
but obtain startEvent instance in the setup and save it for generate in member 
variable. In this case, generate method won't have a chance of accessing 
modified (in this case - removed) data.

Also, private static cache map should go in favor of the store component. 
Otherwise, larger site might simply run out of memory 'cause JXTemplateGenerator 
ate all of it.

Vadim

Mime
View raw message