memory leak in org.mortbay.jetty.servlet.ServletHandler?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

memory leak in org.mortbay.jetty.servlet.ServletHandler?

kevan
I've been chasing some Geronimo leaks (described in http://issues.apache.org/jira/browse/GERONIMO-484)

I've run across the following problem that seems to be a Jetty issue. Comments?

(B) org.apache.commons.logging.LogFactory
  * static field "factories" is holding onto LogFactory objects
  * org.mortbay.jetty.servlet.ServletHandler.doStart() creates new GeronimoLog instances with the following call:
_contextLog = LogFactory.getLog(getHttpContext().getHttpContextName());
  * however, there is no corresponding LogFactory.release(_contextLog) in ServletHandler.doStop()
Reply | Threaded
Open this post in threaded view
|

RE: memory leak in org.mortbay.jetty.servlet.ServletHandler?

Bordet, Simone
Hi

are you saying that the logger created in ServletHandler is the *only*
one that leaks ?

For example, org.mortbay.http.PathMap also instantiates a Log object,
and this class does not seem to have a clear lifecycle like
ServletHandler (no start/stop methods).

So my question is to clarify if all Log instances are already cleared
apart ServletHandler's, or if ServletHandler Log is only the top of the
iceberg.

Thanks,

Simon

________________________________

        From: [hidden email]
[mailto:[hidden email]] On Behalf Of Kevan
Miller
        Sent: Tuesday, August 23, 2005 17:17
        To: [hidden email]
        Subject: [jetty-discuss] memory leak in
org.mortbay.jetty.servlet.ServletHandler?
       
       
        I've been chasing some Geronimo leaks (described in
http://issues.apache.org/jira/browse/GERONIMO-484)
       
        I've run across the following problem that seems to be a Jetty
issue. Comments?
       
        (B) org.apache.commons.logging.LogFactory
          * static field "factories" is holding onto LogFactory objects
          * org.mortbay.jetty.servlet.ServletHandler.doStart() creates
new GeronimoLog instances with the following call:
        _contextLog =
LogFactory.getLog(getHttpContext().getHttpContextName());
          * however, there is no corresponding
LogFactory.release(_contextLog) in ServletHandler.doStop()
       



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
jetty-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jetty-discuss
Reply | Threaded
Open this post in threaded view
|

Re: memory leak in org.mortbay.jetty.servlet.ServletHandler?

Greg Wilkins-5
In reply to this post by kevan

Kevan,

I'm just checking in a version that uses commons logging 1.0.4 and
we are now calling release on the context classloader when the context
is stopped.

HOWEVER

I don't think the example you give below is not a leak, at least not
from Jetty.  There is no mechanism to release a single log, only the log
factories for a classloader.  

The context log is loaded from the LogFactory of the classloader that
loaded Jetty.   So it will be reused over a stop/start of a given context.
If Jetty it self is unloaded, then the container that loaded Jetty should release
the commons log factory that jetty used and thus release that logger.

I'll do an RC1 release shortly and you can test if this is true or not.

cheers


Kevan Miller wrote:

> I've been chasing some Geronimo leaks (described in
> http://issues.apache.org/jira/browse/GERONIMO-484)
>
> I've run across the following problem that seems to be a Jetty issue.
> Comments?
>
> (B) org.apache.commons.logging.LogFactory
>   * static field "factories" is holding onto LogFactory objects
>   * org.mortbay.jetty.servlet.ServletHandler.doStart() creates new
> GeronimoLog instances with the following call:
> _contextLog = LogFactory.getLog(getHttpContext().getHttpContextName());
>   * however, there is no corresponding LogFactory.release(_contextLog)
> in ServletHandler.doStop()



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
jetty-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jetty-discuss
Reply | Threaded
Open this post in threaded view
|

Re: memory leak in org.mortbay.jetty.servlet.ServletHandler?

kevan
Hi Greg,
Apologies for the slow response. I'm not sure I'm in sync with you as to who should be releasing LogFactory's. I'm not sure what your RC1 update will do and what you'd like me to test.

Seems like Geronimo should be calling LogFactory.release() for the case I'm describing. I think Geronimo owns the context/classloader for which the LogFactory is being allocated. And thus Geronimo should be calling LogFactory.release. Do you agree? I've suggested a change to Geronimo (o.a.g.jetty.JettyWebAppContext) that addresses the problem (I may not yet have the appropriate scope for the release, but will be resolving this).

--kevan

On 8/23/05, Greg Wilkins <[hidden email]> wrote:

Kevan,

I'm just checking in a version that uses commons logging 1.0.4 and
we are now calling release on the context classloader when the context
is stopped.

HOWEVER

I don't think the example you give below is not a leak, at least not
from Jetty.  There is no mechanism to release a single log, only the log
factories for a classloader.

The context log is loaded from the LogFactory of the classloader that
loaded Jetty.   So it will be reused over a stop/start of a given context.
If Jetty it self is unloaded, then the container that loaded Jetty should release
the commons log factory that jetty used and thus release that logger.

I'll do an RC1 release shortly and you can test if this is true or not.

cheers


Kevan Miller wrote:

> I've been chasing some Geronimo leaks (described in
> http://issues.apache.org/jira/browse/GERONIMO-484)
>
> I've run across the following problem that seems to be a Jetty issue.
> Comments?
>
> (B) org.apache.commons.logging.LogFactory
>   * static field "factories" is holding onto LogFactory objects
>   * org.mortbay.jetty.servlet.ServletHandler.doStart() creates new
> GeronimoLog instances with the following call:
> _contextLog = LogFactory.getLog(getHttpContext().getHttpContextName());
>   * however, there is no corresponding LogFactory.release(_contextLog)
> in ServletHandler.doStop()


Reply | Threaded
Open this post in threaded view
|

Re: memory leak in org.mortbay.jetty.servlet.ServletHandler?

Greg Wilkins-5

Kevan,

The 5.1.5rc1 release of Jetty has the LogFactory.release() call for it's
own ClassLoader, so it will definitely help with the jcl memory leaks.

It also contains a patch suggested by David Jencks for another object
leak when context are stopped and Jetty did not release it's own components.

As for the context specific jcl created by Jetty, that will be referenced
by the classloader that loaded jetty, so it is geronimos job to release
the jcl factories for the Jetty if/when it removes Jetty from a running
geronimo server.   I'm a bit out of touch with geronimo at the moment,
but I don't think that code will be in o.a.g.jetty.JettyWebAppContext
as the classloader it deals with there will not be the loader that
actaully loaded the Jetty classes themselves.   However the loader
in o.a.g.jetty.JettyWebAppContext should also release jcl factories (and it
does appear to do so).

cheers



Kevan Miller wrote:

> Hi Greg,
> Apologies for the slow response. I'm not sure I'm in sync with you as to
> who should be releasing LogFactory's. I'm not sure what your RC1 update
> will do and what you'd like me to test.
>
> Seems like Geronimo should be calling LogFactory.release() for the case
> I'm describing. I think Geronimo owns the context/classloader for which
> the LogFactory is being allocated. And thus Geronimo should be calling
> LogFactory.release. Do you agree? I've suggested a change to Geronimo
> (o.a.g.jetty.JettyWebAppContext) that addresses the problem (I may not
> yet have the appropriate scope for the release, but will be resolving this).
>
> --kevan
>
> On 8/23/05, *Greg Wilkins* <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>
>     Kevan,
>
>     I'm just checking in a version that uses commons logging 1.0.4 and
>     we are now calling release on the context classloader when the context
>     is stopped.
>
>     HOWEVER
>
>     I don't think the example you give below is not a leak, at least not
>     from Jetty.  There is no mechanism to release a single log, only the log
>     factories for a classloader.
>
>     The context log is loaded from the LogFactory of the classloader that
>     loaded Jetty.   So it will be reused over a stop/start of a given
>     context.
>     If Jetty it self is unloaded, then the container that loaded Jetty
>     should release
>     the commons log factory that jetty used and thus release that logger.
>
>     I'll do an RC1 release shortly and you can test if this is true or not.
>
>     cheers
>
>
>     Kevan Miller wrote:
>     > I've been chasing some Geronimo leaks (described in
>     > http://issues.apache.org/jira/browse/GERONIMO-484)
>     <http://issues.apache.org/jira/browse/GERONIMO-484)>
>     >
>     > I've run across the following problem that seems to be a Jetty issue.
>     > Comments?
>     >
>     > (B) org.apache.commons.logging.LogFactory
>     >   * static field "factories" is holding onto LogFactory objects
>     >   * org.mortbay.jetty.servlet.ServletHandler.doStart() creates new
>     > GeronimoLog instances with the following call:
>     > _contextLog =
>     LogFactory.getLog(getHttpContext().getHttpContextName());
>     >   * however, there is no corresponding LogFactory.release(_contextLog)
>     > in ServletHandler.doStop()
>
>

Reply | Threaded
Open this post in threaded view
|

Re: memory leak in org.mortbay.jetty.servlet.ServletHandler?

kevan
Hi Greg,
Thanks for the info. The LogFactory's that I saw being leaked were for  "org.apache.geronimo.jetty.JettyClassLoader". I assume it's Geronimo's responsibility to clean these up... Geronimo wasn't and o.a.g.jetty.JettyWebAppContext was only recently updated to do this (I assume that this is the LogFactory.release() that you are referring to...).

Finally, if you are referring to the bug in o.m.util.Container.removeComponent(Object) (a bug that I found and David Jenks reported), then I can confirm that the problem has been fixed.

On 8/31/05, Greg Wilkins <[hidden email]> wrote:

Kevan,

The 5.1.5rc1 release of Jetty has the LogFactory.release() call for it's
own ClassLoader, so it will definitely help with the jcl memory leaks.

It also contains a patch suggested by David Jencks for another object
leak when context are stopped and Jetty did not release it's own components.

As for the context specific jcl created by Jetty, that will be referenced
by the classloader that loaded jetty, so it is geronimos job to release
the jcl factories for the Jetty if/when it removes Jetty from a running
geronimo server.   I'm a bit out of touch with geronimo at the moment,
but I don't think that code will be in o.a.g.jetty.JettyWebAppContext
as the classloader it deals with there will not be the loader that
actaully loaded the Jetty classes themselves.   However the loader
in o.a.g.jetty.JettyWebAppContext should also release jcl factories (and it
does appear to do so).

cheers



Kevan Miller wrote:

> Hi Greg,
> Apologies for the slow response. I'm not sure I'm in sync with you as to
> who should be releasing LogFactory's. I'm not sure what your RC1 update
> will do and what you'd like me to test.
>
> Seems like Geronimo should be calling LogFactory.release() for the case
> I'm describing. I think Geronimo owns the context/classloader for which
> the LogFactory is being allocated. And thus Geronimo should be calling
> LogFactory.release. Do you agree? I've suggested a change to Geronimo
> (o.a.g.jetty.JettyWebAppContext) that addresses the problem (I may not
> yet have the appropriate scope for the release, but will be resolving this).
>
> --kevan
>
> On 8/23/05, *Greg Wilkins* <[hidden email]
> <mailto:[hidden email]>> wrote:
>

>
>     Kevan,
>
>     I'm just checking in a version that uses commons logging 1.0.4 and
>     we are now calling release on the context classloader when the context
>     is stopped.
>
>     HOWEVER
>
>     I don't think the example you give below is not a leak, at least not
>     from Jetty.  There is no mechanism to release a single log, only the log
>     factories for a classloader.
>
>     The context log is loaded from the LogFactory of the classloader that
>     loaded Jetty.   So it will be reused over a stop/start of a given
>     context.
>     If Jetty it self is unloaded, then the container that loaded Jetty
>     should release
>     the commons log factory that jetty used and thus release that logger.
>
>     I'll do an RC1 release shortly and you can test if this is true or not.
>
>     cheers
>
>
>     Kevan Miller wrote:
>     > I've been chasing some Geronimo leaks (described in
>     > http://issues.apache.org/jira/browse/GERONIMO-484)
>     <http://issues.apache.org/jira/browse/GERONIMO-484)>
>     >
>     > I've run across the following problem that seems to be a Jetty issue.
>     > Comments?
>     >
>     > (B) org.apache.commons.logging.LogFactory
>     >   * static field "factories" is holding onto LogFactory objects
>     >   * org.mortbay.jetty.servlet.ServletHandler.doStart () creates new
>     > GeronimoLog instances with the following call:
>     > _contextLog =
>     LogFactory.getLog(getHttpContext().getHttpContextName());
>     >   * however, there is no corresponding LogFactory.release(_contextLog)
>     > in ServletHandler.doStop()
>
>