james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Burrell Donkin" <robertburrelldon...@gmail.com>
Subject Re: [mime4j] MultiReferenceStorage Locking
Date Mon, 01 Dec 2008 13:49:52 GMT
On Mon, Dec 1, 2008 at 12:56 PM, Markus Wiederkehr
<markus.wiederkehr@gmail.com> wrote:
> On Mon, Dec 1, 2008 at 8:43 AM, Robert Burrell Donkin
> <robertburrelldonkin@gmail.com> wrote:
>> On 11/30/08, Markus Wiederkehr <markus.wiederkehr@gmail.com> wrote:
>>> On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin
>>> <robertburrelldonkin@gmail.com> 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?

i don't have an easy link for this

(i recommend Doug Lea's

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

the synchronized thread escapes from the scope controlled by the
library through the call. if this can be avoided, it should be since
it open up potential concurrency issues in code that the library does
not control. synchronizing just the reference counting code (as below)
would prevent this possibility.

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

open a new issue, please

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

typically you just do something like:

private final Object referenceCountMonitor = new Object();

and synchronize on that (rather than this). the advantage is that the
monitor is inaccessible which means the concurrency cannot be
influenced by interactions outside the class. the disadvantage is that
it's more complex. unless someone has spotted something i haven't,
it's probably unnecessary in this case.

- robert

To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org

View raw message