svn commit: r1733 - in logback/trunk: logback-classic/src/main/java/ch/qos/logback/classic/jmx logback-core/src/main/java/ch/qos/logback/core logback-core/src/main/java/ch/qos/logback/core/status logback-core/src/main/java/ch/qos/logback/core/util logback-core/src/test/java/ch/qos/logback/core/joran/action logback-core/src/test/java/ch/qos/logback/core/status
Author: ceki Date: Wed Aug 6 18:51:10 2008 New Revision: 1733 Added: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/status/StatusListener.java Modified: logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/jmx/Configurator.java logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java logback/trunk/logback-core/src/main/java/ch/qos/logback/core/status/StatusManager.java logback/trunk/logback-core/src/main/java/ch/qos/logback/core/util/StatusPrinter.java logback/trunk/logback-core/src/test/java/ch/qos/logback/core/joran/action/PropertyActionTest.java logback/trunk/logback-core/src/test/java/ch/qos/logback/core/status/StatusChecker.java Log: Relates to LBCLASSIC-51 LBCLASSIC-59 LBCLASSIC-58 Fixed synchronization problems. The iterator() method has been removed from the StatusManager interface. It has been replaced by getCopyOfStatusList which, as the name indicates, returns a copy of the statusList contained in the StatusManager instance. This avoids concurrency problems encountered when the developer forget to synchronize on the statusManager before iterating on its status list. Added a listener interface to status manager as suggested in LBCLASSIC-59 by Anton Tagunov. Support for status listeners in Joran is still missing. Modified: logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/jmx/Configurator.java ============================================================================== --- logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/jmx/Configurator.java (original) +++ logback/trunk/logback-classic/src/main/java/ch/qos/logback/classic/jmx/Configurator.java Wed Aug 6 18:51:10 2008 @@ -143,7 +143,7 @@ public List<String> getStatuses() { List<String> list = new ArrayList<String>(); - Iterator<Status> it = context.getStatusManager().iterator(); + Iterator<Status> it = context.getStatusManager().getCopyOfStatusList().iterator(); while(it.hasNext()) { list.add(it.next().toString()); } Modified: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java ============================================================================== --- logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java (original) +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java Wed Aug 6 18:51:10 2008 @@ -10,25 +10,39 @@ package ch.qos.logback.core; import java.util.ArrayList; -import java.util.Iterator; +import java.util.Collections; import java.util.List; import ch.qos.logback.core.status.Status; +import ch.qos.logback.core.status.StatusListener; import ch.qos.logback.core.status.StatusManager; public class BasicStatusManager implements StatusManager { public static final int MAX_COUNT = 200; + // This method is synchronized on the instance. + // Code int count = 0; - List<Status> statusList = new ArrayList<Status>(); + + // reading SynchronizedCollection the mutex is the returned + // synchronized list, we make use of this fact in getCopyOfStatusList + List<Status> statusList = Collections + .synchronizedList(new ArrayList<Status>()); int level = Status.INFO; - // This method is synchronized on the instance. - // Code iterating on this.iterator is expected to - // also synchronize on this (the BasicStatusManager instance) - public synchronized void add(Status newStatus) { - // System.out.println(newStatus); + // reading SynchronizedCollection the mutex is the returned + // synchronized list, we make use of this fact in getCopyOfStatusListnerList + List<StatusListener> statusListenerList = Collections + .synchronizedList(new ArrayList<StatusListener>()); + + /** + * Add a new status object. + * + * @param Status + * the status message to add + */ + public void add(Status newStatus) { if (count > MAX_COUNT) { return; } @@ -40,8 +54,10 @@ statusList.add(newStatus); } - public Iterator<Status> iterator() { - return statusList.iterator(); + public synchronized List<Status> getCopyOfStatusList() { + synchronized (statusList) { + return new ArrayList<Status>(statusList); + } } public int getLevel() { @@ -52,4 +68,18 @@ return count; } + public void add(StatusListener listener) { + statusListenerList.add(listener); + } + + public void remove(StatusListener listener) { + statusListenerList.add(listener); + } + + public List<StatusListener> getCopyOfStatusListenerList() { + synchronized (statusListenerList) { + return new ArrayList<StatusListener>(statusListenerList); + } + } + } Added: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/status/StatusListener.java ============================================================================== --- (empty file) +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/status/StatusListener.java Wed Aug 6 18:51:10 2008 @@ -0,0 +1,5 @@ +package ch.qos.logback.core.status; + +public interface StatusListener { + void addStatusEvent(Status status); +} Modified: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/status/StatusManager.java ============================================================================== --- logback/trunk/logback-core/src/main/java/ch/qos/logback/core/status/StatusManager.java (original) +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/status/StatusManager.java Wed Aug 6 18:51:10 2008 @@ -9,16 +9,52 @@ */ package ch.qos.logback.core.status; -import java.util.Iterator; - +import java.util.List; +/** + * Internal error messages (statii) are managed by instances of this interface. + * + * @author Ceki Gulcu + */ public interface StatusManager { + + /** + * Add a new status message. + * + * @param status + */ public void add(Status status); - public Iterator<Status> iterator(); + + /** + * Obtain a copy of the status list maintained by this StatusManager. + * + * @return + */ + public List<Status> getCopyOfStatusList(); + + /** + * Return the highest level of all the statii. + * + * @return + */ public int getLevel(); + /** - * Return the number of entries. + * Return the number of status entries. + * * @return */ public int getCount(); + + public void add(StatusListener listener); + + public void remove(StatusListener listener); + + /** + * Obtain a copy of the status listener list maintained by this StatusManager + * + * @return + */ + public List<StatusListener> getCopyOfStatusListenerList(); + } Modified: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/util/StatusPrinter.java ============================================================================== --- logback/trunk/logback-core/src/main/java/ch/qos/logback/core/util/StatusPrinter.java (original) +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/util/StatusPrinter.java Wed Aug 6 18:51:10 2008 @@ -55,13 +55,10 @@ } public static void buildStr(StringBuilder sb, StatusManager sm) { - - synchronized (sm) { - Iterator it = sm.iterator(); - while (it.hasNext()) { - Status s = (Status) it.next(); - buildStr(sb, "", s); - } + Iterator it = sm.getCopyOfStatusList().iterator(); + while (it.hasNext()) { + Status s = (Status) it.next(); + buildStr(sb, "", s); } } Modified: logback/trunk/logback-core/src/test/java/ch/qos/logback/core/joran/action/PropertyActionTest.java ============================================================================== --- logback/trunk/logback-core/src/test/java/ch/qos/logback/core/joran/action/PropertyActionTest.java (original) +++ logback/trunk/logback-core/src/test/java/ch/qos/logback/core/joran/action/PropertyActionTest.java Wed Aug 6 18:51:10 2008 @@ -83,13 +83,13 @@ } private boolean checkError() { - Iterator it = context.getStatusManager().iterator(); + Iterator it = context.getStatusManager().getCopyOfStatusList().iterator(); ErrorStatus es = (ErrorStatus)it.next(); return PropertyAction.INVALID_ATTRIBUTES.equals(es.getMessage()); } private boolean checkFileErrors() { - Iterator it = context.getStatusManager().iterator(); + Iterator it = context.getStatusManager().getCopyOfStatusList().iterator(); ErrorStatus es1 = (ErrorStatus)it.next(); boolean result1 = "Could not read properties file [toto].".equals(es1.getMessage()); ErrorStatus es2 = (ErrorStatus)it.next(); Modified: logback/trunk/logback-core/src/test/java/ch/qos/logback/core/status/StatusChecker.java ============================================================================== --- logback/trunk/logback-core/src/test/java/ch/qos/logback/core/status/StatusChecker.java (original) +++ logback/trunk/logback-core/src/test/java/ch/qos/logback/core/status/StatusChecker.java Wed Aug 6 18:51:10 2008 @@ -16,42 +16,39 @@ import ch.qos.logback.core.status.Status; import ch.qos.logback.core.status.StatusManager; - public class StatusChecker { StatusManager sm; - + public StatusChecker(StatusManager sm) { this.sm = sm; } - + public boolean containsMatch(String regex) { - + Pattern p = Pattern.compile(regex); - - - Iterator stati = sm.iterator(); - while(stati.hasNext()) { + + Iterator stati = sm.getCopyOfStatusList().iterator(); + while (stati.hasNext()) { Status status = (Status) stati.next(); String msg = status.getMessage(); Matcher matcher = p.matcher(msg); - if(matcher.lookingAt()) { + if (matcher.lookingAt()) { return true; } else { - System.out.println("no match:"+msg); - System.out.println("regex :"+regex); + System.out.println("no match:" + msg); + System.out.println("regex :" + regex); } } return false; } - - + public boolean containsException(Class exceptionType) { - Iterator stati = sm.iterator(); - while(stati.hasNext()) { + Iterator stati = sm.getCopyOfStatusList().iterator(); + while (stati.hasNext()) { Status status = (Status) stati.next(); Throwable t = status.getThrowable(); - if(t != null && t.getClass().getName().equals(exceptionType.getName())) { + if (t != null && t.getClass().getName().equals(exceptionType.getName())) { return true; } }
Hi Ceki, some comments on the latest commit: noreply.ceki@qos.ch wrote:
Modified: logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java ============================================================================== --- logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java (original) +++ logback/trunk/logback-core/src/main/java/ch/qos/logback/core/BasicStatusManager.java Wed Aug 6 18:51:10 2008 @@ -10,25 +10,39 @@
[..]
+ // This method is synchronized on the instance. + // Code int count = 0; - List<Status> statusList = new ArrayList<Status>(); + + // reading SynchronizedCollection the mutex is the returned + // synchronized list, we make use of this fact in getCopyOfStatusList + List<Status> statusList = Collections + .synchronizedList(new ArrayList<Status>());
statusList should be final if it's later used for synchronization. At least that's what IDEA says...
int level = Status.INFO;
- // This method is synchronized on the instance. - // Code iterating on this.iterator is expected to - // also synchronize on this (the BasicStatusManager instance) - public synchronized void add(Status newStatus) { - // System.out.println(newStatus); + // reading SynchronizedCollection the mutex is the returned + // synchronized list, we make use of this fact in getCopyOfStatusListnerList + List<StatusListener> statusListenerList = Collections + .synchronizedList(new ArrayList<StatusListener>());
As statusList, statusListenerList should be final.
+ + /** + * Add a new status object. + * + * @param Status + * the status message to add + */ + public void add(Status newStatus) { if (count > MAX_COUNT) { return; } @@ -40,8 +54,10 @@ statusList.add(newStatus); }
- public Iterator<Status> iterator() { - return statusList.iterator(); + public synchronized List<Status> getCopyOfStatusList() { + synchronized (statusList) { + return new ArrayList<Status>(statusList); + } }
I guess the method shouldn't be synchronized anymore since synchronization is done on statusList. [..]
+ public void remove(StatusListener listener) { + statusListenerList.add(listener); + }
Should be statusListenerList.remove(listener); Regards, Joern.
Joern Huxhorn wrote:
Hi Ceki, some comments on the latest commit:
statusList should be final if it's later used for synchronization. At As statusList, statusListenerList should be final.
OK.
- public Iterator<Status> iterator() { - return statusList.iterator(); + public synchronized List<Status> getCopyOfStatusList() { + synchronized (statusList) { + return new ArrayList<Status>(statusList); + } }
I guess the method shouldn't be synchronized anymore since synchronization is done on statusList.
From what I can read from the ArrayList constructor taking a collection, copying of the collection passed as parameter is not synchronized. Moreover, the mutex used by the SyncronizedArrayList is the object itself (the SyncronizedArrayList instance), so synchronized(statusList){...} is necessary.
+ public void remove(StatusListener listener) { + statusListenerList.add(listener); + }
Should be statusListenerList.remove(listener);
Four eyes are better than two. thank you!
Regards, Joern.
-- Ceki Gülcü
participants (3)
-
Ceki Gulcu -
Joern Huxhorn -
noreply.ceki＠qos.ch