You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2007/04/23 02:32:43 UTC
svn commit: r531306 - in /tomcat:
container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/
container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Author: markt
Date: Sun Apr 22 17:32:43 2007
New Revision: 531306
URL: http://svn.apache.org/viewvc?view=rev&rev=531306
Log:
Fix some logging related memory leaks. This fixes 41272 and the root cause of 41939.
Modified:
tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java
tomcat/container/tc5.5.x/webapps/docs/changelog.xml
tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java
Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java?view=diff&rev=531306&r1=531305&r2=531306
==============================================================================
--- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java (original)
+++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java Sun Apr 22 17:32:43 2007
@@ -168,6 +168,8 @@
super();
+ log = LogFactory.getLog(ApplicationDispatcher.class);
+
// Save all of our configuration parameters
this.wrapper = wrapper;
this.context = (Context) wrapper.getParent();
@@ -191,7 +193,7 @@
// ----------------------------------------------------- Instance Variables
- private static Log log = LogFactory.getLog(ApplicationDispatcher.class);
+ private Log log = null;
/**
* The Context this RequestDispatcher is associated with.
Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java
URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java?view=diff&rev=531306&r1=531305&r2=531306
==============================================================================
--- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java (original)
+++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java Sun Apr 22 17:32:43 2007
@@ -62,7 +62,7 @@
*/
public abstract class ManagerBase implements Manager, MBeanRegistration {
- protected Log log = LogFactory.getLog(ManagerBase.class);
+ protected Log log = null;
// ----------------------------------------------------- Instance Variables
@@ -692,11 +692,14 @@
Registry.getRegistry(null, null).unregisterComponent(oname);
initialized=false;
oname = null;
+ log = null;
}
public void init() {
if( initialized ) return;
initialized=true;
+
+ log = LogFactory.getLog(ManagerBase.class);
if( oname==null ) {
try {
Modified: tomcat/container/tc5.5.x/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/webapps/docs/changelog.xml?view=diff&rev=531306&r1=531305&r2=531306
==============================================================================
--- tomcat/container/tc5.5.x/webapps/docs/changelog.xml (original)
+++ tomcat/container/tc5.5.x/webapps/docs/changelog.xml Sun Apr 22 17:32:43 2007
@@ -67,6 +67,10 @@
org.apache.catalina.loader.WebappClassLoader.ENABLE_CLEAR_REFERENCES to
false will stop these fields being set to null on context stop. (markt)
</fix>
+ <fix>
+ Fix a logging related memory leak in ManagerBase and
+ ApplicationDispatcher. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Webapps">
@@ -160,6 +164,9 @@
<fix>
<bug>42072</bug> Don't call destroy() if the associated init() fails.
Patch provided by Kawasima Kazuh. (markt)
+ </fix>
+ <fix>
+ Fix a logging related memory leak in PageContextImpl. (markt)
</fix>
</changelog>
</subsection>
Modified: tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java
URL: http://svn.apache.org/viewvc/tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java?view=diff&rev=531306&r1=531305&r2=531306
==============================================================================
--- tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java (original)
+++ tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java Sun Apr 22 17:32:43 2007
@@ -101,8 +101,6 @@
* Constructor.
*/
PageContextImpl(JspFactory factory) {
- log = LogFactory.getLog(getClass());
-
this.variableResolver = new VariableResolverImpl(this);
this.outs = new BodyContentImpl[0];
this.attributes = new Hashtable(16);
@@ -130,6 +128,8 @@
boolean autoFlush) throws IOException {
// initialize state
+ log = LogFactory.getLog(getClass());
+
this.servlet = servlet;
this.config = servlet.getServletConfig();
this.context = config.getServletContext();
@@ -199,6 +199,8 @@
session = null;
attributes.clear();
+
+ log = null;
}
public Object getAttribute(final String name) {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r531306 - in /tomcat: container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Posted by Remy Maucherat <re...@apache.org>.
Mark Thomas wrote:
> Remy Maucherat wrote:
>> markt@apache.org wrote:
>>> public abstract class ManagerBase implements Manager,
>>> MBeanRegistration {
>>> - protected Log log = LogFactory.getLog(ManagerBase.class);
>>> + protected Log log = null;
>> I'm also fairly certain this causes NPEs, so this part of the change has
>> to be reverted as well (for example, calling setRandomFile). This issue
>> is good to fix, as I have pointed out in the bug report.
>
> I did check this and couldn't see a code path that would call this
> prior to init() being called. What am I missing?
This can be called very simply, by trying to set a randomFile using
server.xml.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r531306 - in /tomcat: container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> markt@apache.org wrote:
>> public abstract class ManagerBase implements Manager,
>> MBeanRegistration {
>> - protected Log log = LogFactory.getLog(ManagerBase.class);
>> + protected Log log = null;
>
> I'm also fairly certain this causes NPEs, so this part of the change has
> to be reverted as well (for example, calling setRandomFile). This issue
> is good to fix, as I have pointed out in the bug report.
I did check this and couldn't see a code path that would call this
prior to init() being called. What am I missing?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r531306 - in /tomcat: container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Posted by Remy Maucherat <re...@apache.org>.
markt@apache.org wrote:
> Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java
> URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java?view=diff&rev=531306&r1=531305&r2=531306
> ==============================================================================
> --- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java (original)
> +++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java Sun Apr 22 17:32:43 2007
> @@ -62,7 +62,7 @@
> */
>
> public abstract class ManagerBase implements Manager, MBeanRegistration {
> - protected Log log = LogFactory.getLog(ManagerBase.class);
> + protected Log log = null;
I'm also fairly certain this causes NPEs, so this part of the change has
to be reverted as well (for example, calling setRandomFile). This issue
is good to fix, as I have pointed out in the bug report.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r531306 - in /tomcat: container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Posted by Remy Maucherat <re...@apache.org>.
markt@apache.org wrote:
> Modified: tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java
> URL: http://svn.apache.org/viewvc/tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java?view=diff&rev=531306&r1=531305&r2=531306
> ==============================================================================
> --- tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java (original)
> +++ tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java Sun Apr 22 17:32:43 2007
> @@ -101,8 +101,6 @@
> * Constructor.
> */
> PageContextImpl(JspFactory factory) {
> - log = LogFactory.getLog(getClass());
> -
> this.variableResolver = new VariableResolverImpl(this);
> this.outs = new BodyContentImpl[0];
> this.attributes = new Hashtable(16);
> @@ -130,6 +128,8 @@
> boolean autoFlush) throws IOException {
>
> // initialize state
> + log = LogFactory.getLog(getClass());
> +
I didn't see it a few hours ago (it was late), but this change has the
same problem as the change for the application dispatcher, so it is also
not good. This issue is about fighting against a windmill: it will
continue happening with any class which is not loaded before starting
the application, or if the logger is acquired as part of the object
creation if it's an instance field. Given that I consider these changes
to not be acceptable, you cannot possibly fix this non issue.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r531306 - in /tomcat: container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Posted by Remy Maucherat <re...@apache.org>.
Mark Thomas wrote:
> Remy Maucherat wrote:
>> markt@apache.org wrote:
>>> Fix some logging related memory leaks. This fixes 41272 and the root
>> -1. Getting a logger has, AFAIK, a possibly significant cost. Since
>> getting a RD is a very common option, this is not acceptable.
> I took a look at the commons-logging code. The first call for a
> context will be expensive as the logger has to be created. After that
> it is:
> - one call to getContextClassLoader() (via reflection)
> - two hashtable look ups
> - some wrapper code
> I knew it used getContextClassLoader(). I didn't realise it was via
> reflection so I agree with you about the performance hit.
This is still far more than what should happen (nothing). I did provide
suggestions to write an equivalent patch which would not suffer from the
problem, so I maintain my veto.
>> If you really want to fix this non issue (which will simply happen
>> elsewhere in more insidious form),
> I don't view memory leaks and loggers for one context logging messages
> for a different context a non-issue. Given all it takes for this to
> occur is to put log4j in a webapp this is likely to affect a lot of
> people - including me ;). I am happy to spend the time fixing these
> issues and any others that crop up in the future.
All I'm asking in the end if you really want to do it for some reason is
that you find a mutually acceptable way.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r531306 - in /tomcat: container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> markt@apache.org wrote:
>> Fix some logging related memory leaks. This fixes 41272 and the root
> -1. Getting a logger has, AFAIK, a possibly significant cost. Since
> getting a RD is a very common option, this is not acceptable.
I took a look at the commons-logging code. The first call for a
context will be expensive as the logger has to be created. After that
it is:
- one call to getContextClassLoader() (via reflection)
- two hashtable look ups
- some wrapper code
I knew it used getContextClassLoader(). I didn't realise it was via
reflection so I agree with you about the performance hit.
> If you really want to fix this non issue (which will simply happen
> elsewhere in more insidious form),
I don't view memory leaks and loggers for one context logging messages
for a different context a non-issue. Given all it takes for this to
occur is to put log4j in a webapp this is likely to affect a lot of
people - including me ;). I am happy to spend the time fixing these
issues and any others that crop up in the future.
> I think you should find another way (removing a
> lot of the logging would be good, since it's lame debug which should not
> be there, and maybe use wrapper.getLogger).
I like the wrapper.getLogger() idea. I don't have time to look at this
tonight - I'll take a look tomorrow.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r531306 - in /tomcat: container/tc5.5.x/catalina/src/share/org/apache/catalina/core/
container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ container/tc5.5.x/webapps/docs/
jasper/tc5.5.x/src/share/org/apache/jasper/runtime/
Posted by Remy Maucherat <re...@apache.org>.
markt@apache.org wrote:
> Author: markt
> Date: Sun Apr 22 17:32:43 2007
> New Revision: 531306
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=531306
> Log:
> Fix some logging related memory leaks. This fixes 41272 and the root cause of 41939.
>
> Modified:
> tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
> tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/ManagerBase.java
> tomcat/container/tc5.5.x/webapps/docs/changelog.xml
> tomcat/jasper/tc5.5.x/src/share/org/apache/jasper/runtime/PageContextImpl.java
>
> Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
> URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java?view=diff&rev=531306&r1=531305&r2=531306
> ==============================================================================
> --- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java (original)
> +++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java Sun Apr 22 17:32:43 2007
> @@ -168,6 +168,8 @@
>
> super();
>
> + log = LogFactory.getLog(ApplicationDispatcher.class);
> +
> // Save all of our configuration parameters
> this.wrapper = wrapper;
> this.context = (Context) wrapper.getParent();
> @@ -191,7 +193,7 @@
>
> // ----------------------------------------------------- Instance Variables
>
> - private static Log log = LogFactory.getLog(ApplicationDispatcher.class);
> + private Log log = null;
>
> /**
> * The Context this RequestDispatcher is associated with.
-1. Getting a logger has, AFAIK, a possibly significant cost. Since
getting a RD is a very common option, this is not acceptable. If you
really want to fix this non issue (which will simply happen elsewhere in
more insidious form), I think you should find another way (removing a
lot of the logging would be good, since it's lame debug which should not
be there, and maybe use wrapper.getLogger).
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org