<div dir="ltr"><br><br><div class="gmail_quote">On Wed, Aug 6, 2008 at 10:00 PM, Ceki Gulcu <span dir="ltr"><listid@qos.ch></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d"><br>
<br>
Joern Huxhorn wrote:<br>
> Hi Ceki,<br>
> some comments on the latest commit:<br>
<br>
</div><div class="Ih2E3d">> statusList should be final if it's later used for synchronization. At<br>
</div><div class="Ih2E3d">> As statusList, statusListenerList should be final.<br>
<br>
</div>OK.<br>
<div class="Ih2E3d">>><br>
>> - public Iterator<Status> iterator() {<br>
>> - return statusList.iterator();<br>
>> + public synchronized List<Status> getCopyOfStatusList() {<br>
>> + synchronized (statusList) {<br>
>> + return new ArrayList<Status>(statusList);<br>
>> + }<br>
>> }<br>
>><br>
> I guess the method shouldn't be synchronized anymore since<br>
> synchronization is done on statusList.<br>
<br>
</div> From what I can read from the ArrayList constructor taking a collection,<br>
copying of the collection passed as parameter is not synchronized. Moreover, the<br>
mutex used by the SyncronizedArrayList is the object itself (the<br>
SyncronizedArrayList instance), so synchronized(statusList){...} is necessary.<br>
<div class="Ih2E3d"></div></blockquote><div><br>Yes, definitely @ synchronized(statusList).<br>I just think that <br>public synchronized List<Status> getCopyOfStatusList()<br>should be<br>public List<Status> getCopyOfStatusList() <br>
so there is no risk of deadlock.<br><br>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.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br>
>> + public void remove(StatusListener listener) {<br>
>> + statusListenerList.add(listener);<br>
>> + }<br>
>><br>
> Should be statusListenerList.remove(listener);<br>
<br>
</div>Four eyes are better than two. thank you!<br>
</blockquote><div><br>Your welcome,<br>Joern.</div></div><br></div>