Locking dilema

[Mark Smyth]

Hi guys,
First off sorry about the long post and also for starting yet another locking thread. I would like to ask your opinions on my locking strategy of which I have two different versions. Before I start I must point out record number is a place number in the file including deleted records (Which are never returned). So a findAll() might return 1, 4,5,8,9.as active records for example.
The first version synchronizes on a HashMap but the notifyAll() on this in the unlocking method wakes up all threads including those not interested in the record that has just been unlocked. The consumes little enough space as only currently locked methods are stored in this HashMap. The code looks like this

HashMap LockedRecords;
public void lock(int recNo) throws RecordNotFoundException{
        synchronized(lockedRecords){
         while( isLocked(recNo) ){
                try{
                    lockedRecords.wait();
                }
                catch(InterruptedException ie){}
            }
        lockedRecords.put(new Integer(recNo),  Thread.currentThread().getName());
        }
}

The second maintains an ArrayList of lock objects for both deleted and active records, but of course only active records will actually lock. This is much less scalable (bad always) but guarantees that only interested threads are woken (good for SCJD). This ArrayList object is only ever altered by int create() (only if a record is appended to end of file) and an initialiselocks() method in the Data object constructor. In these methods I synchronize on the container.
As it is only modified by these two methods I believe the following code to be thread safe (Synchronization in locking methods is done on the specific record element get(recNo) only). Seemed ok in tests anyway, is more efficient but certainly less scalable.
I couldn�t think of a way to do this with a hashmap as you cannot synchronize on a null object (If the record is not locked) and besides you would have to synchronize on the HashMap in the lock methods as it is being altered. Currently the hashmap impl is commented out but I think that I would be more comfortable about submitting the it than this one.
Off the point somewhat, ironically I left out an almost identical caching mechanism for pretty much the same reason, except in that case I didn�t really know how to achieve a better result. But I am now thinking if it�s a bad idea for a cache, it�s a bad idea for locks.
Here is my second lock method

 
private ArrayList locks; Instance var
public void lock(int recNo) throws RecordNotFoundException{
        //Initialise stuff, Check record exists 
LockObject  theLock=(LockObject)locks.get(recNo)
        synchronized(theLock){
            while( theLock.isLocked()){
                try{
                    theLock.wait();
                }
                catch(InterruptedException ie){}
            }
            theLock.lock(Thread.currentThread().getName());
        }
    } 

This is the lock initialisation method called in Data(). Instance variable numRecords and numFields used in body.
private void initaliseLocks(RandomAccessFile file) throws IOException{
}
And this is the bones of LockObject stored in the ArrayList just contains a Boolean and a String for the name, simple implementation omitted.

 
private class LockObject{
    private String threadName;
    private boolean isLocked;
    synchronized boolean isLockedBy(String name){ }
    synchronized boolean  isLocked(){}
    public String getName(){ }
    synchronized void lock(String name){ }
    synchronized void  unlock(String name){ }
}

Also perhaps as the second method is a little more complex perhaps it goes against the simplicity before efficiency spirit of the assignment (In my spec anyway).
I would really appreciate any comments or suggestions on the pros and cons of each approach.

[Satish Avadhanam]

Hi Mark,

Originally posted by Mark Smyth:
Hi guys,
First off sorry about the long post and also for starting yet another locking thread. I would like to ask your opinions on my locking strategy of which I have two different versions. Before I start I must point out record number is a place number in the file including deleted records (Which are never returned). So a findAll() might return 1, 4,5,8,9.as active records for example.
The first version synchronizes on a HashMap but the notifyAll() on this in the unlocking method wakes up all threads including those not interested in the record that has just been unlocked. The consumes little enough space as only currently locked methods are stored in this HashMap. The code looks like this

HashMap LockedRecords;
public void lock(int recNo) throws RecordNotFoundException{
        synchronized(lockedRecords){
         while( isLocked(recNo) ){
                try{
                    lockedRecords.wait();
                }
                catch(InterruptedException ie){}
            }
        lockedRecords.put(new Integer(recNo),  Thread.currentThread().getName());
        }
}

The second maintains an ArrayList of lock objects for both deleted and active records, but of course only active records will actually lock. This is much less scalable (bad always) but guarantees that only interested threads are woken (good for SCJD).

I have to admit Mark, I'm lost here. Here you say that only active records will lock and that's exactly what is needed. But what I don't understand is - what is the difference in this and the above one where you are synchronizing the lockedRecords HashMap. In the lockedRecords also only active records will be added right? So on what terms are you differentiating this from the above approach?

This ArrayList object is only ever altered by int create() (only if a record is appended to end of file) and an initialiselocks() method in the Data object constructor.

Here, what do you mean by initializing locks? Before the first thread access Data object or when the first thread access Data, why are you populating the initializelocks? What are you storing in them? It seems that you are storing all the records present, both active and deleted from DB. Am I right?

In these methods I synchronize on the container.
As it is only modified by these two methods I believe the following code to be thread safe (Synchronization in locking methods is done on the specific record element get(recNo) only). Seemed ok in tests anyway, is more efficient but certainly less scalable.
I couldn�t think of a way to do this with a hashmap as you cannot synchronize on a null object (If the record is not locked) and besides you would have to synchronize on the HashMap in the lock methods as it is being altered. Currently the hashmap impl is commented out but I think that I would be more comfortable about submitting the it than this one.
Off the point somewhat, ironically I left out an almost identical caching mechanism for pretty much the same reason, except in that case I didn�t really know how to achieve a better result. But I am now thinking if it�s a bad idea for a cache, it�s a bad idea for locks.
Here is my second lock method

 
private ArrayList locks; Instance var
public void lock(int recNo) throws RecordNotFoundException{
        //Initialise stuff, Check record exists 
LockObject  theLock=(LockObject)locks.get(recNo)
        synchronized(theLock){
            while( theLock.isLocked()){
                try{
                    theLock.wait();
                }
                catch(InterruptedException ie){}
            }
            theLock.lock(Thread.currentThread().getName());
        }
    } 

This is the lock initialisation method called in Data(). Instance variable numRecords and numFields used in body.
private void initaliseLocks(RandomAccessFile file) throws IOException{
}

Same as above. I do not understand what you are doing in this method. Are you populating all the active and deleted records in lockedRecords HashMap before the very first thread access the DB?

And this is the bones of LockObject stored in the ArrayList just contains a Boolean and a String for the name, simple implementation omitted.

 
private class LockObject{
    private String threadName;
    private boolean isLocked;
    synchronized boolean isLockedBy(String name){ }
    synchronized boolean  isLocked(){}
    public String getName(){ }
    synchronized void lock(String name){ }
    synchronized void  unlock(String name){ }
}

Also perhaps as the second method is a little more complex perhaps it goes against the simplicity before efficiency spirit of the assignment (In my spec anyway).
I would really appreciate any comments or suggestions on the pros and cons of each approach.

Mark, I would like to help and learn. But as you can see, obviously I'm lost
I cannot say "Hope this helps" now as I think am also confusing you too

[Philippe Maquet]

Hi Mark,
Second solution is original, and I personally like it, but both solutions are OK. Less scalable? Which scalability? In terms of number of concurrent clients (good definition IMO) or of volume of data (which should stay reasonable anyway in such an application) ?
In fact, such a locking solution could be combined with a simple and efficient cache I think, sharing the *same* ArrayList: just imagine this:
your LockObject (null if record is deleted) wrapped in some RecInfo object (stored in your ArrayList), along with some CacheRecord which itself stores a record (String[]) as a SoftReference. Just an idea.
Best,
Phil.

[Mark Smyth]

Sorry perhaps I should clarify a bit more. The hashmap is initally empty. The first method places the String threadname and the key recordNumber in the Hashmap to lock a record. Say record 2 and 3 are locked. And there happens to be 2 threads waiting to lock these 2 records. When lock on 2 is releases both the threads waiting are awakened the thread waiting for record 3 unnesscessarly so.
The second method hasa arraylist containing a lockobject for every active and deleted record in the file. The initialise method simply adds a lock object for each record active and deleted to the arraylist. The index of the arraylist is equivelant to the record number. When you synchronize on the lockobject you check the islocked boolean to see if it is locked or not. The difference in this approach is that if as the previous example threads are waiting for 2 and 3 and 2 is unlocked, only the thread wating on 2 is woken.
Sorry about the confusion only my english programming aint too good, hope this clears it up.

[Mark Smyth]

In fact, such a locking solution could be combined with a simple and efficient cache I think, sharing the *same* ArrayList: just imagine this:
your LockObject (null if record is deleted) wrapped in some RecInfo object (stored in your ArrayList), along with some CacheRecord which itself stores a record (String[]) as a SoftReference. Just an idea.
Best,

I am using a value object in my middle tier, perhaps I could modify the LockObject to contain a reference to a cached VO. This already has a String[] getRecordData() method declared so I could use this to comply with the DBMain interface specifications for the return value of read and it could just return the record string array if it exists in that way I suppose.
That is a great idea Philippe I will definately give it some more thought. Thanks very much

[Satish Avadhanam]

Hi Mark,

Originally posted by Mark Smyth:
Sorry perhaps I should clarify a bit more. The hashmap is initally empty. The first method places the String threadname and the key recordNumber in the Hashmap to lock a record. Say record 2 and 3 are locked. And there happens to be 2 threads waiting to lock these 2 records. When lock on 2 is releases both the threads waiting are awakened the thread waiting for record 3 unnesscessarly so.

Yes, I understood this one before but was confused by the second method. Thanks for clarifying again.

The second method hasa arraylist containing a lockobject for every active and deleted record in the file. The initialise method simply adds a lock object for each record active and deleted to the arraylist. The index of the arraylist is equivelant to the record number. When you synchronize on the lockobject you check the islocked boolean to see if it is locked or not. The difference in this approach is that if as the previous example threads are waiting for 2 and 3 and 2 is unlocked, only the thread wating on 2 is woken.

Yep, this is a very good idea which I could'nt grasp at first Now that you explained so clearly, I need to think about this. My present approach is same as your first.

Sorry about the confusion only my english programming aint too good, hope this clears it up.

Thanks