On Wed, Aug 6, 2008 at 10:00 PM, Ceki Gulcu <listid@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.