cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Torsten Curdt <tcu...@apache.org>
Subject Re: Javaflow - major memory issue
Date Sun, 30 Mar 2008 11:35:09 GMT

On Mar 30, 2008, at 09:43, Joerg Heinicke wrote:
> On 28.03.2008 04:55, Torsten Curdt wrote:
>
>>> The output I showed pointed to  
>>> org.apache.cocoon.components.flow.java.Continuation which only  
>>> seems to exist in Cocoon 2.1. Nothing unsets the context there.
>> Hah - well spotted!
>>> Having a look into the code continuations are only handled by  
>>> JavaInterpreter. There are two methods callFunction(..) and  
>>> handleContinuation(..) calling Continuation.registerThread() and  
>>> deregisterThread() in a finally block. From a brief look I have no  
>>> idea if I can just unset the ContinuationContext there as well.  
>>> You might know more about it.
>> We should add a try/finally block in Continuation.suspend() that  
>> clears the context after a suspend. That should fix it.
>
> Unfortunately, it is not possible to unset the ContinuationContext  
> completely. It's needed in JavaInterpreter.handleContinuation(..)  
> when a child continuation is created:
>
>  Continuation parentContinuation =
>      (Continuation) parentwk.getContinuation();
>  ContinuationContext parentContext =
>      (ContinuationContext) parentContinuation.getContext();
>  ContinuationContext context = new ContinuationContext();
>  context.setObject(parentContext.getObject());
>  context.setMethod(parentContext.getMethod());

There is no need to really obtain that value from the parent. If  
handleContinuation would have also the function parameter it could use  
the same initialization as in callFuntion. Actually a way of fixing  
this would be to add two Strings to the Continuation class. One  
holding the classname, the other holding the method name. And then do

context.setObject(userScopes.get(parentContinuation.getScopeName()));
context.setMethod(methods.get(parentContinuation.getFunctionName()));

in handleContinuation. Then the cleaning of the context should be fine.

> Without completely rewriting it the only thing I did was to remove  
> the data in the ContinuationContext that is not necessary. I do this  
> by an extra call to ContinuationContext.onSuspend() in  
> AbstractContinuable since Continuation is not aware of the  
> implementation of its context (it's just an Object).
>
> Please review my changes [1] because I'm not really sure about them.

Not a fan of the Collections.synchronizedMap(new HashMap()) change -  
but other than that they look OK on the first glance :)

> They work for the normal case, but what happens in an error case? I  
> can't see what's really going on except that the method is left on  
> Continuation.suspend() ... It was very interesting to debug it when  
> AbstractContinuable.sendPageAndWait(..) was actually hit twice.

:) ...what error case do you mean?

> I guess this handling is different in 2.2. There a clean  
> ContinuationContext is created on both callFunction(..) and  
> handleContinuation(..).

Indeed ...that would be another fix ...porting it from 2.2 :)

Thanks for looking into that.

cheers
--
Torsten

Mime
View raw message