On Mon, Dec 1, 2008 at 8:43 AM, Robert Burrell Donkin <[EMAIL PROTECTED]> wrote: > On 11/30/08, Markus Wiederkehr <[EMAIL PROTECTED]> wrote: >> On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin >> <[EMAIL PROTECTED]> wrote: >>> http://svn.apache.org/repos/asf/james/mime4j/trunk/src/main/java/org/apache/james/mime4j/message/storage/MultiReferenceStorage.java >>> uses method based synchronization to protect the reference counting >>> variable. i wonder whether this is the best approach. (i tend to >>> prefer explicit monitors that cannot leak for defensive reasons.) >>> >>> in particular >>> >>> public synchronized void delete() { >>> if (referenceCounter == 0) >>> return; >>> >>> referenceCounter--; >>> >>> if (referenceCounter == 0) { >>> storage.delete(); >>> } >>> } >>> >>> 1. storage is an interface and so storage.delete() represents a leak >>> of the thread of execution. i wonder whether it is possible for >>> competing storage deletes to produce a deadlock. >>> 2. storage.delete() may be a slow operation. the locking appears to be >>> protecting only the reference counting. if so then the delete may not >>> need to be synchronized, >>> >>> it might be possible to avoid explicit synchronization by using >>> AtomicInteger instead >>> >>> opinions? >> >> Robert, thank you very much for committing my patches so fast. >> >> Regarding the problem with the interface and the leak you describe, I >> think I have to read up on that one.. > > Basically, the class has no control over the implementation and cannot > stop deadlock conditions etc
I'm sorry I don't quite understand that, do you have a link that describes the problem? The way I see it if a method is synchronized it means that an instance of the class gets locked when the method gets invoked. In our case an instance of MultiReferenceStorage gets locked. This object also happens to implement the Storage interface but where's the problem with that? The same happens with java.util.Set for example. Interface Set itself is not synchronized but there might be implementations that are. Collections.synchronizedSet() returns a wrapper that does the synchronizing. This is very similar to what I have done with MultiReferenceStorage. >> But for delete() possibly being a slow operation, how about using >> internaynchronization instead? I mean determine if the storage has >> to be deleted in a "synchronized this" block and do the actual >> deletion outside of the block.. > > +1 Okay, I can submit a patch if you want. Should I attach it to MIME4J-88 or open a new issue? > It's also worthwhile thinking about whether it would be better to use > a different monitor (rather than this). Not sure whether the > additional complex would be a good trade in this case. I could write a class called Counter that encapsules the integer, for instance. Counter could behave similar to AtomicInteger. But that would establish a one-to-one relationship between MultiReferenceStorage and Counter so I don't see any advantage in using Counter for synchronizing. Markus --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
