You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by Robert Burrell Donkin <ro...@gmail.com> on 2008/11/30 23:59:13 UTC
[mime4j] MultiReferenceStorage Locking
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
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 11/30/08, Markus Wiederkehr <ma...@gmail.com> wrote:
> On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin
> <ro...@gmail.com> wrote:
>> it might be possible to avoid explicit synchronization by using
>> AtomicInteger instead
>
> Sounds good but AtomicInteger is a Java 5 feature that would introduce
> a dependency on backport-util-concurrent.jar in the retrotranslated
> jar files. The same kind of dependency I wanted to avoid by not making
> o.a.j.mime4j.message.Mode a native Java 5 enum..
Ok makes sense
- robert
>
> Markus
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Markus Wiederkehr <ma...@gmail.com>.
On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin
<ro...@gmail.com> wrote:
> it might be possible to avoid explicit synchronization by using
> AtomicInteger instead
Sounds good but AtomicInteger is a Java 5 feature that would introduce
a dependency on backport-util-concurrent.jar in the retrotranslated
jar files. The same kind of dependency I wanted to avoid by not making
o.a.j.mime4j.message.Mode a native Java 5 enum..
Markus
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Mon, Dec 1, 2008 at 5:49 PM, Markus Wiederkehr
<ma...@gmail.com> wrote:
> On Mon, Dec 1, 2008 at 2:49 PM, Robert Burrell Donkin
> <ro...@gmail.com> wrote:
>> On Mon, Dec 1, 2008 at 12:56 PM, Markus Wiederkehr
>> <ma...@gmail.com> wrote:
>>> 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.
>
> Do I understand correctly that something like this would be okay?
>
> public void delete() {
> if (decrementCounter()) {
> storage.delete();
> }
> }
>
> private synchronized boolean decrementCounter() {
> return --referenceCounter == 0;
> }
>
> Because although "this" is used as monitor the object is no longer
> locked when storage.delete() gets invoked and the thread escapes from
> the scope controlled by mime4j?
yes - so long as the call is outside the synchronized block, the
thread won't escape from scope
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Markus Wiederkehr <ma...@gmail.com>.
On Mon, Dec 1, 2008 at 2:49 PM, Robert Burrell Donkin
<ro...@gmail.com> wrote:
> On Mon, Dec 1, 2008 at 12:56 PM, Markus Wiederkehr
> <ma...@gmail.com> wrote:
>> 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.
Do I understand correctly that something like this would be okay?
public void delete() {
if (decrementCounter()) {
storage.delete();
}
}
private synchronized boolean decrementCounter() {
return --referenceCounter == 0;
}
Because although "this" is used as monitor the object is no longer
locked when storage.delete() gets invoked and the thread escapes from
the scope controlled by mime4j?
Markus
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Markus Wiederkehr <ma...@gmail.com>.
On Mon, Dec 1, 2008 at 2:49 PM, Robert Burrell Donkin
<ro...@gmail.com> wrote:
> On Mon, Dec 1, 2008 at 12:56 PM, Markus Wiederkehr
> <ma...@gmail.com> wrote:
>> 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
> http://www.amazon.com/Concurrent-Programming-Java-Principles-Patterns/dp/0201695812)
Interesting, thanks!
Markus
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On Mon, Dec 1, 2008 at 12:56 PM, Markus Wiederkehr
<ma...@gmail.com> wrote:
> On Mon, Dec 1, 2008 at 8:43 AM, Robert Burrell Donkin
> <ro...@gmail.com> wrote:
>> On 11/30/08, Markus Wiederkehr <ma...@gmail.com> wrote:
>>> On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin
>>> <ro...@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
http://www.amazon.com/Concurrent-Programming-Java-Principles-Patterns/dp/0201695812)
> 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
Re: [mime4j] MultiReferenceStorage Locking
Posted by Markus Wiederkehr <ma...@gmail.com>.
On Mon, Dec 1, 2008 at 8:43 AM, Robert Burrell Donkin
<ro...@gmail.com> wrote:
> On 11/30/08, Markus Wiederkehr <ma...@gmail.com> wrote:
>> On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin
>> <ro...@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?
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: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 11/30/08, Markus Wiederkehr <ma...@gmail.com> wrote:
> On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin
> <ro...@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
> 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
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.
- Robert
>
> Markus
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [mime4j] MultiReferenceStorage Locking
Posted by Markus Wiederkehr <ma...@gmail.com>.
On Sun, Nov 30, 2008 at 11:59 PM, Robert Burrell Donkin
<ro...@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..
But for delete() possibly being a slow operation, how about using
internal synchronization 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..
Markus
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org