Here you have my comment on this revision: big commit -> big review
LockableObjectStore: not sure the name is right, you are not locking the OS but only a given key.
I would use lock(Serializable key) and unlock(Serializable key) instead of lockEntry and releaseEntry. (or acquire/release)
LockEntry's javadoc is wrong on this part "read/update this key". Should say read/remove this key as there is no update operation in a OS.
What happens when there is no object in the store with that key? That must be also defined in the javadoc.
RelaseEntry: what happens if the key is not locked or is locked on a different thread? that info should be on the javadoc.
DefaultLockableObjectStore looks more like a LockableObjectStoreAdapter, ie, a way to convert a standard OS into a lockable one.
DefaultLockableObjectStore must lock/release each key before operating on it, otherwise the lock mechanism is useless.
The usage of the "this" keyword is redundant in many places.
MuleEntryLocker: name must be more generic because this interface can be used to lock anything, not just "entries.
Javadoc must be improved to define the contract more explicitly.
After seeing this interface Is not clear to me why you need to define a LockableObjectStore if you can just implement MuleEntryLocker in DefaultLockableObjectStore
acquireLock must be final to avoid possible bugs.
Remove the redundant usage of "this"
The lock/release schema seems weak to me: any call to release will work without checking if the caller is the thread that holds the lock. I would expect this methods work similar to how the lock/unlock methods work on ReentrantLock.
I think there are a couple of race conditions on MuleServerEntryLockerTestCase when the key is not in the store:
1) The worker thread does lockableObjectStore.lock(key); and then it does a lockableObjectStore.release(key); on the finally block. In this case there will be an exception when the thread did not locked the key (what not in the store) but another thread locked it.
2) The worker thread does objectStore.contains(key) and then objectStore.store(key,value + 1);. In this case there will be an exception when the object was already added in a different thread
Seems to me that the API must be extended in some way, maybe the lock method should create a lock even when the key is not there in order to avoid the race condition.
On Fri, Aug 17, 2012 at 4:04 PM, <[hidden email]> wrote:
This post has NOT been accepted by the mailing list yet.
Any CDEV doc that explains the rationale for LockableObjectStore?
On Sat, Aug 18, 2012 at 3:59 PM, Pablo Kraan [via Mule] <[hidden email]> wrote:
In reply to this post by Pablo Kraan
Hi Pablo LG,
The added test needs to be reviewed ASAP: it broke http://bamboo.mulesoft.org/browse/MULE33X-JDK6-202/ and hung
On Sat, Aug 18, 2012 at 7:59 PM, Pablo Kraan <[hidden email]> wrote:
I'm going to revert all my changes around ObjectStore. For 3.3.1 I will only provide a locking mechanism and use that directly from IdempotentRedeliveryPolicy.
ObjectStore hierarchy is a mess. I prefer not to add anything related to locking until we refactor ObjectStore implementation.
On Mon, Aug 20, 2012 at 7:21 PM, Pablo Kraan <[hidden email]> wrote:
Hi Pablo LG,
|Free forum by Nabble||Edit this page|