Author: kkolinko Date: Mon Dec 13 02:09:32 2010 New Revision: 1045003 URL: http://svn.apache.org/viewvc?rev=1045003&view=rev Log: Improve the fix for https://issues.apache.org/bugzilla/show_bug.cgi?id=50201 - Use AtomicReference to guard against possible duplicate registration of listeners when logAccess() is called by several threads at the same time. - Added install() and uninstall() method to AccessLogListener to help register/unregister the listeners. - When defaultAccessLog is nulled, always unregister the listener as well. The listener does not need to be reused, because now we do create a listener when there is no log and it will take care of things.
Modified: tomcat/trunk/java/org/apache/catalina/core/StandardEngine.java tomcat/trunk/webapps/docs/changelog.xml Modified: tomcat/trunk/java/org/apache/catalina/core/StandardEngine.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardEngine.java?rev=1045003&r1=1045002&r2=1045003&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/core/StandardEngine.java (original) +++ tomcat/trunk/java/org/apache/catalina/core/StandardEngine.java Mon Dec 13 02:09:32 2010 @@ -19,6 +19,7 @@ package org.apache.catalina.core; import java.beans.PropertyChangeEvent; import java.beans.PropertyChangeListener; import java.util.Locale; +import java.util.concurrent.atomic.AtomicReference; import org.apache.catalina.AccessLog; import org.apache.catalina.Container; @@ -115,7 +116,8 @@ public class StandardEngine extends Cont * Default access log to use for request/response pairs where we can't ID * the intended host and context. */ - private volatile AccessLog defaultAccessLog; + private final AtomicReference<AccessLog> defaultAccessLog = + new AtomicReference<AccessLog>(); // ------------------------------------------------------------- Properties @@ -321,7 +323,7 @@ public class StandardEngine extends Cont } if (!logged && useDefault) { - AccessLog newDefaultAccessLog = defaultAccessLog; + AccessLog newDefaultAccessLog = defaultAccessLog.get(); if (newDefaultAccessLog == null) { // If we reached this point, this Engine can't have an AccessLog // Look in the defaultHost @@ -331,22 +333,25 @@ public class StandardEngine extends Cont newDefaultAccessLog = host.getAccessLog(); if (newDefaultAccessLog != null) { - AccessLogListener l = new AccessLogListener(this); - this.addPropertyChangeListener(l); - host.addContainerListener(l); - host.addLifecycleListener(l); + if (defaultAccessLog.compareAndSet(null, + newDefaultAccessLog)) { + AccessLogListener l = new AccessLogListener(this, + host, null); + l.install(); + } } else { // Try the ROOT context of default host context = (Context) host.findChild(""); if (context != null && context.getState().isAvailable()) { newDefaultAccessLog = context.getAccessLog(); - if (newDefaultAccessLog != null) { - AccessLogListener l = - new AccessLogListener(this); - this.addPropertyChangeListener(l); - context.addLifecycleListener(l); + if (defaultAccessLog.compareAndSet(null, + newDefaultAccessLog)) { + AccessLogListener l = new AccessLogListener( + this, null, context); + l.install(); + } } } } @@ -354,18 +359,13 @@ public class StandardEngine extends Cont if (newDefaultAccessLog == null) { newDefaultAccessLog = new NoopAccessLog(); - AccessLogListener l = new AccessLogListener(this); - this.addPropertyChangeListener(l); - if (host != null) { - host.addContainerListener(l); - host.addLifecycleListener(l); - } - if (context != null) { - context.addLifecycleListener(l); + if (defaultAccessLog.compareAndSet(null, + newDefaultAccessLog)) { + AccessLogListener l = new AccessLogListener(this, host, + context); + l.install(); } - } - defaultAccessLog = newDefaultAccessLog; } newDefaultAccessLog.log(request, response, time); @@ -407,29 +407,53 @@ public class StandardEngine extends Cont ContainerListener { private StandardEngine engine; + private Host host; + private Context context; private volatile boolean disabled = false; - public AccessLogListener(StandardEngine engine) { + public AccessLogListener(StandardEngine engine, Host host, + Context context) { this.engine = engine; + this.host = host; + this.context = context; + } + + public void install() { + engine.addPropertyChangeListener(this); + if (host != null) { + host.addContainerListener(this); + host.addLifecycleListener(this); + } + if (context != null) { + context.addLifecycleListener(this); + } + } + + private void uninstall() { + disabled = true; + if (context != null) { + context.removeLifecycleListener(this); + } + if (host != null) { + host.removeLifecycleListener(this); + host.removeContainerListener(this); + } + engine.removePropertyChangeListener(this); } @Override public void lifecycleEvent(LifecycleEvent event) { if (disabled) return; - + String type = event.getType(); if (Lifecycle.AFTER_START_EVENT.equals(type) || - Lifecycle.BEFORE_STOP_EVENT.equals(type)) { - // Container is being started/stopped - // Force re-calculation but do not disable listener since it - // might be re-used - engine.defaultAccessLog = null; - } else if (Lifecycle.BEFORE_DESTROY_EVENT.equals(type)) { - // Container is being removed + Lifecycle.BEFORE_STOP_EVENT.equals(type) || + Lifecycle.BEFORE_DESTROY_EVENT.equals(type)) { + // Container is being started/stopped/removed // Force re-calculation and disable listener since it won't // be re-used - engine.defaultAccessLog = null; - disabled = true; + engine.defaultAccessLog.set(null); + uninstall(); } } @@ -439,8 +463,8 @@ public class StandardEngine extends Cont if ("defaultHost".equals(evt.getPropertyName())) { // Force re-calculation and disable listener since it won't // be re-used - engine.defaultAccessLog = null; - disabled = true; + engine.defaultAccessLog.set(null); + uninstall(); } } @@ -448,13 +472,13 @@ public class StandardEngine extends Cont public void containerEvent(ContainerEvent event) { // Only useful for hosts if (disabled) return; - if (Container.ADD_CHILD_EVENT.equals(event.getType())) { Context context = (Context) event.getData(); if ("".equals(context.getPath())) { - // New ROOT context in default host - // Force recalculation of default access log - engine.defaultAccessLog = null; + // Force re-calculation and disable listener since it won't + // be re-used + engine.defaultAccessLog.set(null); + uninstall(); } } } Modified: tomcat/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1045003&r1=1045002&r2=1045003&view=diff ============================================================================== --- tomcat/trunk/webapps/docs/changelog.xml (original) +++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec 13 02:09:32 2010 @@ -81,6 +81,11 @@ options to control the <code>SecureRandom</code> instances used to generate session IDs. (markt) </update> + <fix> + <bug>50201</bug>: Update the access log reference in + <code>StandardEngine</code> when the ROOT web application is redeployed, + started, stopped or defaultHost is changed. (markt/kkolinko) + </fix> <add> <bug>50282</bug>: Load <code>javax.security.auth.login.Configuration</code> with --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org