[logback-dev] 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/lo

[Jörn Huxhorn]

On Wed, Aug 6, 2008 at 10:00 PM, Ceki Gulcu <listid at qos.ch> wrote:

>
>
> 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.
>

Yes, definitely @ synchronized(statusList).
I just think that
public synchronized List<Status> getCopyOfStatusList()
should be
public List<Status> getCopyOfStatusList()
so there is no risk of deadlock.

I haven't checked if there *is* a risk of deadlock but there *could* be
under certain circumstances, i.e. if there would be another place that
synchronizes on statusList first and on Object later. I don't think there is
such a piece of code but it's generally a good idea to not
synchronize/holding two locks if not absolutely necessary. It's also a bit
faster.


> >> +  public void remove(StatusListener listener) {
> >> +    statusListenerList.add(listener);
> >> +  }
> >>
> > Should be statusListenerList.remove(listener);
>
> Four eyes are better than two. thank you!
>

Your welcome,
Joern.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://qos.ch/pipermail/logback-dev/attachments/20080806/6ebacc9c/attachment.htm