I made a slight adjustment to the pre-Java-5 solution and replaced it
(unconditionally here, though this could easily be made conditional)
with a Java 5 implementation.
Jess Holle wrote:
We as many others have been bitten by the synchronization nuances of
log4j.
Attached is a proposed patch to reduce the scope of the problem. No,
this is not yet regression tested due to the issues I e-mailed about
previously with the regression test suite, but I'd like to spur some
discussion as to what's wrong with such a change, etc.
I was considering using Java 5 features here and could, but the key
issue here does not appear to require such fanciness to address.
--
Jess Holle
# This patch file was generated by NetBeans IDE
# Following Index: paths are relative to: D:\User
Profiles\jessh.PTCNET\Desktop\log4jsvn
# This patch can be applied using context Tools: Patch action on respective
folder.
# It uses platform neutral UTF-8 encoding and \n newlines.
# Above lines and this line are ignored by the patching process.
Index: src/main/java/org/apache/log4j/Category.java
--- src/main/java/org/apache/log4j/Category.java Base (BASE)
+++ src/main/java/org/apache/log4j/Category.java Locally Modified (Based On
LOCAL)
@@ -36,7 +36,7 @@
import org.apache.log4j.spi.LoggerRepository;
import org.apache.log4j.spi.HierarchyEventListener;
import org.apache.log4j.helpers.NullEnumeration;
-import org.apache.log4j.helpers.AppenderAttachableImpl;
+import org.apache.log4j.helpers.AppenderAttachableImpl5;
import java.util.Enumeration;
import java.util.MissingResourceException;
@@ -123,7 +123,7 @@
protected LoggerRepository repository;
- AppenderAttachableImpl aai;
+ AppenderAttachableImpl5 aai;
/** Additivity is set to true by default, that is children inherit
the appenders of their ancestors by default. If this variable is
@@ -159,7 +159,7 @@
public
void addAppender(Appender newAppender) {
if(aai == null) {
- aai = new AppenderAttachableImpl();
+ aai = new AppenderAttachableImpl5();
}
aai.addAppender(newAppender);
repository.fireAddAppenderEvent(this, newAppender);
@@ -200,16 +200,15 @@
int writes = 0;
for(Category c = this; c != null; c=c.parent) {
- // Protected against simultaneous call to addAppender, removeAppender,...
- synchronized(c) {
- if(c.aai != null) {
- writes += c.aai.appendLoopOnAppenders(event);
+ // copy into field, so null check and usage are against same value
+ AppenderAttachableImpl5 cAai = c.aai;
+ if (cAai != null) {
+ writes += cAai.appendLoopOnAppenders(event);
}
- if(!c.additive) {
+ if (!c.additive) {
break;
}
}
- }
if(writes == 0) {
repository.emitNoAppenderWarning(this);
Index: src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl.java
--- src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl.java Base
(BASE)
+++ src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl.java Locally
Modified (Based On LOCAL)
@@ -53,21 +53,29 @@
}
/**
- Call the <code>doAppend</code> method on all attached appenders. */
+ Call the <code>doAppend</code> method on all attached appenders.
+ Implementations should be thread safe against other methods on this class.
+ */
public
int appendLoopOnAppenders(LoggingEvent event) {
- int size = 0;
- Appender appender;
-
- if(appenderList != null) {
- size = appenderList.size();
+ // copy into field, so null check and usage are against same value
+ Vector appenderList = this.appenderList;
+ if ( appenderList != null ) {
+ Appender appenders[];
+ synchronized ( appenderList ) {
+ int size = appenderList.size();
+ if ( size == 0 )
+ return 0;
+ appenders = (Appender[]) appenderList.toArray( new Appender[size] );
+ }
+ int size = appenders.length;
for(int i = 0; i < size; i++) {
- appender = (Appender) appenderList.elementAt(i);
- appender.doAppend(event);
+ appenders[i].doAppend(event);
}
- }
return size;
}
+ return 0;
+ }
/**
Index: src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl5.java
--- src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl5.java Locally
New
+++ src/main/java/org/apache/log4j/helpers/AppenderAttachableImpl5.java Locally
New
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.log4j.helpers;
+
+import org.apache.log4j.spi.AppenderAttachable;
+import org.apache.log4j.spi.LoggingEvent;
+import org.apache.log4j.Appender;
+
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+/**
+ A straightforward CopyOnWriteArrayList-based implementation of the
+ [EMAIL PROTECTED] AppenderAttachable} interface.
+ <p>
+ Note that althought the implementation uses CopyOnWriteArrayList (and thus
+ requires Java 5, on the whole it is no more thread safe than
+ AppenderAttachableImpl (which was not thread-safe despite using Vector).
+ The main benefit of using CopyOnWriteArrayList is that
+ appendLoopOnAppenders(LoggingEvent) no longer requires <i>any</i>
+ synchronization or array copying.
+ <p>
+ This class is final simply as I see no need to extend it at this time.
+
+ @author Jess Holle
+ */
+public final class AppenderAttachableImpl5 implements AppenderAttachable {
+
+ /** Array of appenders. */
+ private CopyOnWriteArrayList<Appender> appenderList;
+
+ /**
+ Attach an appender. If the appender is already in the list in
+ won't be added again.
+ */
+ public
+ void addAppender(Appender newAppender) {
+ // Null values for newAppender parameter are strictly forbidden.
+ if(newAppender == null)
+ return;
+
+ if(appenderList == null)
+ appenderList = new CopyOnWriteArrayList<Appender>();
+ if(!appenderList.contains(newAppender))
+ appenderList.add(newAppender);
+ }
+
+ /**
+ Call the <code>doAppend</code> method on all attached appenders.
+ This method is thread-safe against other methods on this class.
+ */
+ public
+ int appendLoopOnAppenders(LoggingEvent event) {
+ // copy into field, so null check and usage are against same value and to
reduce field access
+ CopyOnWriteArrayList<Appender> appenderList = this.appenderList;
+ if ( ( appenderList != null ) && !appenderList.isEmpty() ) {
+ int size = 0;
+ for ( Appender appender : appenderList ) {
+ appender.doAppend(event);
+ ++size;
+ }
+ return size;
+ }
+ return 0;
+ }
+
+ /**
+ Get all attached appenders as an Enumeration. If there are no
+ attached appenders <code>null</code> is returned.
+
+ @return Enumeration An enumeration of attached appenders.
+ */
+ public
+ Enumeration getAllAppenders() {
+ if(appenderList == null)
+ return null;
+ else
+ return Collections.enumeration( appenderList );
+ }
+
+ /**
+ Look for an attached appender named as <code>name</code>.
+
+ <p>Return the appender with that name if in the list. Return null
+ otherwise.
+
+ */
+ public
+ Appender getAppender(String name) {
+ if(appenderList == null || name == null)
+ return null;
+
+ for ( Appender appender : appenderList )
+ if(name.equals(appender.getName()))
+ return appender;
+ return null;
+ }
+
+ /**
+ Returns <code>true</code> if the specified appender is in the
+ list of attached appenders, <code>false</code> otherwise.
+
+ @since 1.2 */
+ public
+ boolean isAttached(Appender appender) {
+ if(appenderList == null || appender == null)
+ return false;
+
+ for ( Appender a : appenderList )
+ if(a == appender)
+ return true;
+ return false;
+ }
+
+ /**
+ * Remove and close all previously attached appenders.
+ * */
+ public
+ void removeAllAppenders() {
+ if(appenderList != null) {
+ for ( Appender a : appenderList )
+ a.close();
+ appenderList.clear();
+ appenderList = null;
+ }
+ }
+
+ /**
+ Remove the appender passed as parameter form the list of attached
+ appenders. */
+ public
+ void removeAppender(Appender appender) {
+ if(appender == null || appenderList == null)
+ return;
+ appenderList.remove(appender);
+ }
+
+ /**
+ Remove the appender with the name passed as parameter form the
+ list of appenders.
+ */
+ public
+ void removeAppender(String name) {
+ if(name == null || appenderList == null)
+ return;
+ for ( Appender appender : appenderList )
+ if(name.equals(appender.getName())) {
+ appenderList.remove(appender);
+ break;
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
