On Wed, Aug 6, 2008 at 10:00 PM, Ceki Gulcu <[EMAIL PROTECTED]> 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.
_______________________________________________
logback-dev mailing list
[email protected]
http://qos.ch/mailman/listinfo/logback-dev
