Re: Possible Leak in MyFaces Orchestrea

[Simon Kitching <sk...@apache.org>]

Hi Steve,

I've created a bugreport for Orchestra to track this:
   http://issues.apache.org/jira/browse/ORCHESTRA-35

I think your suggestion of using the HttpSessionListener interface is a
good one. Unless someone objects, I will update Orchestra's
ConversationManagerSessionListener to implement this interface and
handle sessionDestroyed directly rather than relying on the container. I
can't see how it could cause problems for other containers. Iterating
over the attrs is a minor performance hit for apps with very large
numbers of http-session attributes, but if the webpp is so complex then
this loop is unlikely to be significant.

I will try to find time to do this in the next few days.

There are a couple of other minor Orchestra fixes that it would be nice
to get addressed, so it is probably time to look at getting an
orchestra-3.1 release out in the next month or so.

Thanks for your very clear problem report..

Regards,
Simon

Steve Ronderos schrieb:
>
> Simon,
>
> Sorry about the top-post I wasn't thinking (and my email client is a
> piece...).
>
> I too looked around for the spec on what should happen when a session
> times out.  We are using OC4J :-( .  I'll file a bug report with them.  
>
> There was a workaround I was toying with that could be added to
> Orchestra if you think it is valuable.
>
> I was essentially going to add an HttpSessionListener that removes
> attributes when the session times out.
>
> *public* *class* AttributeRemovalSessionListener *implements*
> HttpSessionListener {
>
>   *public* *void* sessionCreated(HttpSessionEvent se) {}
>
>   *public* *void* sessionDestroyed(HttpSessionEvent se) {
>     Enumeration<String> e = _se.getSession().getAttributeNames()_;
>    *while* (e.hasMoreElements()) {
>       se.getSession().removeAttribute(e.nextElement());
>     }
>   }
> }
>
> With proper exception handling I believe that these methods and the
> HttpSessionListener interface could instead be added to the
> ConversationManagerSessionListener.  I have tested the above listener
> in OC4J and it works, if you think it is worth looking into I could
> test it out on some other Containers to make sure that it doesn't make
> a mess of things.  
>
> Do you think this kind of solution is worth investigating?  Otherwise
> I can look at other workarounds.
>
> Thanks,
>
> Steve Ronderos
>
>
> From: 	Simon Kitching <sk...@apache.org>
> To: 	MyFaces Discussion <us...@myfaces.apache.org>
> Date: 	12/31/2008 10:03 AM
> Subject: 	Re: Possible Leak in MyFaces Orchestrea
>
>
> ------------------------------------------------------------------------
>
>
>
> Hi Steve,
>
> First, PLEASE do not top-post (ie put your reply at the top of an email)
> when someone has previously used bottom-posting. It is really annoying
> and makes the email almost impossible to read sensibly.
>  See:  http://en.wikipedia.org/wiki/Posting_style
>
> I've double-checked the servlet spec, and while I can't find explicit
> wording that says that session timeout triggers removeAttribute on all
> top-level attributes of the session, the docs here do imply it:
> http://java.sun.com/javaee/5/docs/api/javax/servlet/http/HttpSessionBindingListener.html
>
> Apache Tomcat is the servlet engine I use mostly, and it certainly does
> do this. So a bugreport to your servlet-container vendor is probably a
> good idea.
>
> I'm happy to add a workaround in orchestra for this problem, though, if
> we can find one. I don't see how adding HttpSessionBindingListener to
> ConversationManager will help though; that will mean the
> ConversationManager needs to be able to obtain a reference to the
> ConversationManagerSessionListener which is not easily done.
>
> Possibly the ConversationManagerSessionListener could add a dummy object
> into each session, and this dummy object can then implement
> HttpSessionBindingListener and contain a reference to the
> ConversationManagerSessionListener. It probably needs to be transient
> though (should't be distributed in clustered environments). And it
> somehow also needs to handle session passivation/activation correctly.
>
> If you can create a suitable patch for this issue, I would be happy to
> review and apply it. Otherwise I'll try to find some time to come up
> with a solution but it won't be for at least a few weeks.
>
> By the way, what servlet container are you using (not that crappy
> Websphere I hope; it's riddled with non-spec-compliant behaviour...)
>
> Regards,
> Simon
>
> On Tue, 2008-12-30 at 10:43 -0600, Steve Ronderos wrote:
> >
> > Simon,
> >
> > Thanks for responding!
> >
> > I didn't know about the ConversationManagerSessionListener, after
> > poking at it for a little, I think I understand how it all works now.
> >  Unfortunately I'm still experiencing the leak.  
> >
> > I see in the Listener that it removes the ConversationManager objects
> > in the method attributeRemoved.  Is it required for an HttpSession to
> > remove it's attributes and therefore cause attributeRemoved to be
> > called?  I believe the web container we are using does not cause
> > attributeRemoved to be called.  Inside of the container we use when
> > the session is invalidating the attributes are searched for instances
> > of HttpSessionBindingListener.  Each instance that is found has its
> > valueUnbound method called.  If I'm interpreting this correctly, that
> > means that for ConversationManager objects not to leak in my
> > container, they will need to implement this HttpSessionBindingListener
> > interface and remove themselves from the ConversationWiperThread in
> > the valueUnbound method.  Or the container would have to call
> > removeAttribute when the session times out.
> >
> > Does this sound correct?
> >
> > At this point do you think it is an issue with my web container that I
> > should file with the vendor? Or is this something that needs to change
> > in Orchestra to accommodate the container.
> >
> > Thanks,
> >
> > Steve Ronderos
> >
> >
> > From:
> > Simon Kitching
> > <sk...@apache.org>
> > To:
> > MyFaces Discussion
> > <us...@myfaces.apache.org>
> > Date:
> > 12/27/2008 03:59 AM
> > Subject:
> > Re: Possible Leak in MyFaces
> > Orchestrea
> >
> >
> > ______________________________________________________________________
> >
> >
> >
> > On Tue, 2008-12-23 at 10:30 -0600, Steve Ronderos wrote:
> > >
> > > Hello Orchestra Users,
> > >
> > > I posted the following message to the developers mailing list a few
> > > weeks ago and had no response.
> > >
> > > I was wondering if anyone has any information on a potential memory
> > > leak that I see in Orchestra 1.2.
> > >
> > > It appears to me that conversationManagers in
> > > ConversationWiperThread.java gets new ConversationManager objects
> > > added to it but they are only removed through some serialization
> > > method (I don't fully understand the distributed serialization stuff
> > > since I have never used it).  I think that this leak is pretty
> > > small... on the order of 10s of bytes per ConversationManager, but
> > for
> > > long lasting high traffic apps that could eventually become a
> > problem.
> > > Is there something that I have overlooked that ensures that these
> > are
> > > cleaned up? Is this an accepted shortcoming of Orchestra?  Should I
> > > file a JIRA issue?
> >
> > Hi Steve,
> >
> > I don't believe there is a problem here.
> >
> > There is one ConversationManager instance per http-session. It gets
> > created when needed (when something calls
> > ConversationManager.getInstance) and is deleted when the http-session
> > gets deleted.
> >
> > There is one ConversationWiperThread instance per webapp. It never
> > creates or destroys ConversationManager instances. However it does
> > peek
> > inside them in order to destroy Conversation and ConversationContext
> > objects that timeout.
> >
> > Unfortunately, there is no javax.servlet api for getting a list of all
> > the HttpSession objects. Therefore in order for the
> > ConversationWiperThread to know what ConversationManager objects
> > exist,
> > the ConversationManagerSessionListener class registers itself as a
> > SessionListener object on the webapp, and detects when a
> > ConversationManager object is added to an HttpSession.
> >
> > The ConversationWiperThread does add these references into a non-weak
> > map, so if they were never removed from the map that would indeed be a
> > leak - when the http session is destroyed there would still be a
> > reference to the ConversationManager.
> >
> > However if you look at ConversationManagerSessionListener you will see
> > that when an http session is destroyed, the ConversationManager
> > instance
> > (if any) in that session is removed from the wiper-thread's map.
> >
> > So as far as I know, there is no leak.
> >
> > There is also one other issue: the container can "passivate" a session
> > (write it out to disk to free up memory). In this case, we also need
> > to
> > remove the ConversationManager from the wiper-thread. The
> > sessionWillPassivate and sessionDidActivate methods in
> > ConversationManagerSessionListener should handle that case too.
> >
> > If you see any other way in which a memory leak can occur, please let
> > us
> > know.
> >
> > Regards,
> > Simon
> >
> >
> >
>
>
>