B&S Locking Strategy Soundness

[Johnny Hastings]

Alright, I've been through this code 100 times, and I think I have it. Can anyone take a look and tell me if they see any holes in it? I will do a thorough test once I get the Data.java class finished, but it would nice to have a heads-up in case something didn't look kewl. Any thoughts or ideas are appreciated!

From Data.java (specified by Sun in the assignment)

    
/**
* Locks a record so that it can only be updated or deleted by
* a specific client. If the specified record is already locked
* by a different client, the current thread gives up the CPU
* and consumes no CPU cycles until the record is unlocked.
* @param p_recordNumber The record to lock.
* @return A cookie that must be used when the record is
* unlocked, updated, or deleted.
* @throws <code>RecordNotFoundException</code> if the
* specified record does not exist, or if it is flagged as
* deleted.
*/
public long lock(int p_recordNumber)
        throws RecordNotFoundException {
 
    /* Generate unique lock cookie. */
    Long lockCookie = generateLockCookie();
 
    synchronized (this) {
String[] lockRecord;
 
try {
/* Be sure specified record is valid. */
// call read method to get the actual record, set it to
                // in the lockRecord variable. Read will throw an exception
                // if the record is invalid.
} catch (RecordNotFoundException rnfe) {
removeLockCookie(lockCookie);
throw rnfe;
}
 
/* Attempt to lock the record. */
lockHandler.lock(lockRecord, lockCookie);
    }
 
    return lockCookie.longValue();
}

From LockHandler.java. Singleton class I implemented to take care of locks.

    
/**
 * Locks an object to a specific lock cookie. If the specified
 * object is already locked, the current thread gives up the
 * CPU and consumes no CPU cycles until the object is
 * available. If the thread is continually interrupted, an
 * exception will be thrown.
 * @param object The object to lock.
 * @param cookie The cookie to lock the object on.
 * @throws <code>InterruptedException</code> if the thread is
 * continually interrupted while awaiting control of the object
 * to lock.
 */
public void lock(Object lockObject,
    Long lockCookie) throws InterruptedException {
 
    synchronized (lockObject) {
boolean isLocked = false;
int interruptedCount = 0;
 
while (true) {
    // set isLocked by seeing if lockObject is in the locks HashMap
 
    if (!isLocked) {
// put the lockObject into the HashMap of locks.
break;
    } else {
try {
    // wait on the lockObject
        } catch (InterruptedException ie) {
    interruptedCount++;
 
    /* If we're continually interrupted (10 or more times),
throw error to exit. */
}
    }
}
    }
}
 

[Roy Mallard]

OK I've had a quick look at it...kind of hard to understand since you don't describe your locking strategy.
That synchronized (lockObject) line looks a bit suspicious. Are you sure there will only ever be one lockObject object for each record? What happens to the lockObjects when a record is deleted/created?

I assume your generateLockCookie() method is thread safe, and you do some sort of timestamp check to detect if a record has been changed by another client since this client did a search/read.

How are you dealing with deadlocks? If you are using a timeout, what happens if a client loses the lock when they are in the middle of a record modifying method? Your deleteRecord and modifyRecord methods will need some sort of thread protection in that case, to avoid overwriting a newer version with an older version.

[Peter Jakobsen]

You are using a HashMap to store locks. HashMap is not thread safe, so you need to exclusive access, when inserting locks in your lock handler. From what I can see, it looks like to threads can lock different records, and thereby modify the HashMap simultanously.

Regarding deadlocks(when two threads a is mutually waiting on each other), asuming that each client holds at most one lock, which imo is a fair asumption based on the usecases. You should be able to implement your server without the risk of deadlocks.

Stale locks might be a problem depending on the design of your client-server communication. If your client has to first call lock, then update and finally unlock on the server. You have to implement some strategy for dealing with stale locks (when clients 'die' while holding a lock).

[Ales Moukhine]

Hi.
I agree with Roy:

Originally posted by Roy Mallard:
OK I've had a quick look at it...kind of hard to understand since you don't describe your locking strategy.
That synchronized (lockObject) line looks a bit suspicious. Are you sure there will only ever be one lockObject object for each record? What happens to the lockObjects when a record is deleted/created?

and Peter :

Originally posted by Peter Jakobsen :
You are using a HashMap to store locks. HashMap is not thread safe, so you need to exclusive access, when inserting locks in your lock handler. From what I can see, it looks like to threads can lock different records, and thereby modify the HashMap simultanously.

May be it would be better do synchronize in lockHandler.lock() on hashmap?

Next, I don't understand why do you have synchronized block in Data.lock() for?

Regards.
Ales'

[Adam Nace]

Okay, I didn't spend very much time looking it over, but it looks to me like your locking strategy is NOT re-entrant.

What happens if the client that already holds the lock attempts to lock it again? Typically, this should be allowed. Furthermore, you might want to track the number of "entrances", so that that the lock must be "unlocked" the same number of times. (i.e. every lock() and unlock() call must occur in pairs).

I have a bigger criticism though. In the interface that I was provided to do the assignment, the lock method signature was supposed to return void, and the unlock, update, and delete methods did not take a long value cookie parameter. Do you have a different interface which does, or have you modified the interface that was provided? If so, is that allowed? In the instructions I got, I understood that Data.java had to exactly implement the provided interface (and presumably, provide the entire required functionality specified by the comments for each method. i.e. without requiring clients to know about extra methods).

- Adam

[Roy Mallard]

The lack of a cookie parameter should not cause any problems. Remember a method can access any of its classes instance variables.

[Adam Nace]

yeah, but that's ugly. That requires you to synchronize on the entire Data object to first set the instance variable, then do what ever it is you need to do with the object.

besides, if I understand it right, the idea is that you should be able to control your Data object using exclusively the DBMain interface. Otherwards, what would be the point of using the DBMain interface?

[Johnny Hastings]

Thanks for the responses fellas. I guess I should have been a bit more descriptive in my original post.

The DB.java interface I received from Sun does indeed have the lock method returning a type long, not void.

To say a bit more about my locking strategy:
I have a class which implements the provided DB.java interface, called Data.java (just as Sun described in the assignment). That class has a data member which is an instance of the singleton class LockHandler.java. LockHandler is supposed to control all of my locking. LockHandler cares nothing about records or data, just locks. Data.java generates the lock cookies through a synchronized method. It also keeps track of which lock cookies are in use (via an array), and will not use the same lock cookie twice.
I planned on not having clients hold a lock for any time, other than the brief moment of update. If a client wants to update a record, they make their changes, then when it's time to save, the data is sent to the server. @ that point, the database is updated. If an update is currently in process on the current record, then the user waits. This does create the issue of stale data. While one user is working with a Should I reconsider this approach? It seemed like it would satisfy the requirements as long as the proper messages were passed back to the user in case they were trying to book an already booked record. Thoughts on this subjust (as well as on the locking strategy above) are greatly appreciated.