Lock Manager implementation

[Fox Shen]

Hi
I implemented some method for LockManager， just as following:

 
public class LockManager {
    private HashMap hm = new HashMap();
    private boolean dbLock = false;
    public synchronized void registerConnection(int record, Object data) {
        if (record == -1) {
            dbLock = true;
        }
        if (!dbLock && !isConnectionRegistered(record)) {
            hm.put(new Integer(record), data);
        }
        else {
            try {
                data.wait();
            }
            catch (InterruptedException e) {
            }
        }
    }
    public boolean isConnectionRegistered(int record) {
        if (dbLock) {
            return true;
        }
        return hm.containsKey(new Integer(record));
    }
    public synchronized void unregisterConnection(int record, Object data) {
        Object conn = hm.get(new Integer(record));
        if (conn != null && conn == data) {
            hm.remove(new Integer(record));
            data.notifyAll();
        }
    }
}

Now the problem is:
1. Whether I should implement the wait() and notifyAll() in the LockManager？
My consideration is that if we implement them in RemoteData class, we need to change the lock() method's signature to synchronized, that's not i wanted.
2. Is my implementation for lock and unlock, especially for database lock corrcet? PLS comment.
3. As Michael mentioned in:

For the same reason that Data is not. If you can have multiple Data objects representing different tables then you must have a different LockManager for each table. So making LockManager a singleton causes the same maintainance problem as making Data a singleton.

So I plan to write some class named LockManagerFactory to create singleton LockManager for each Data instance. Is that necessary?
Any comment would be appreciated!
Thanks,
--Fox

[Michael Morris]

Hi Fox,

1. Whether I should implement the wait() and notifyAll() in the LockManager？
My consideration is that if we implement them in RemoteData class, we need to change the lock() method's signature to synchronized, that's not i wanted.

The whole purpose of the LockManager is to protect Data from concurrent modification by utilizing a simple write lock, so yes the synchronization should occur in the LockManager class. How you use the acquired lock depends on your ovarall design. In my implementation the remote connection object "played nice" by verifying it actually had the lock before performing a volatile operation on Data (ie modify(), etc.). You could put that responsibility in Data but that requires a tighter coupling between LockManager and Data. My reasoning was that since my remote connection had references to both Data and LockManager, there was no need for Data to participate or even know there was such a thing as a LockManager or remote connection object. To wrap up, you are correct in not wanting to change the lock()/unlock() signatures on your public Data interface and there is no need to do so.

2. Is my implementation for lock and unlock, especially for database lock corrcet? PLS comment.

I don't really understand why you think it is necessary to call wait() and notifAll() on Data. You may have a good reason. If so please explain. I just called wait() and notifyAll() on the LockManager itself.
Also, you have a potential dead lock situation when you release the lock. The notifyAll() should not be conditional. What if a notifed client calls the unregisterConnection() method and for whatever reason the if condition fails? All remaining clients are stuck in the wait pool.

So I plan to write some class named LockManagerFactory to create singleton LockManager for each Data instance. Is that necessary?

There is certainly nothing wrong with that approach. I used a builder pattern and kept a static protected map of LockManagers in my DataBuilder class. Actually, I think your way is more elegant. If you are using a DataFactory similar to what I did, you could even create the LockManager at the first request of a client for each Data object. You could have a method like LockManager getLockManager(Data data) in the DataFactory.
Hope this helps,
Michael Morris

[Fox Shen]

Thanks so much for your detailed reply, Michael.
Now I'm more confident on my LockManager implementation.

My reasoning was that since my remote connection had references to both Data and LockManager, there was no need for Data to participate or even know there was such a thing as a LockManager or remote connection object.

Now my code for RemoteData is as following.

  
    public void lock(int record) throws IOException {
        lockManager.registerConnection(record, this);
    }
    public void unlock(int record) {
        lockManager.unregisterConnection(record, this);
    }

Is that the way you mean "there was no need for Data to participate or even know there was such a thing as a LockManager"?

In my implementation the remote connection object "played nice" by verifying it actually had the lock before performing a volatile operation on Data (ie modify(), etc.).

I'm not so clear about your mean "verifying it actually had the lock before performing a volatile operation on Data", can you give some details for that?

I don't really understand why you think it is necessary to call wait() and notifAll() on Data. You may have a good reason. If so please explain.

I really want to call wait() and notifyAll() on the LockManager itself, but aftering seeing so many person call the wait() and notifyAll() in the Data class, I just lost my confidence. So thanks for your explanation.

Also, you have a potential dead lock situation when you release the lock.

Yes, thanks for the reminder. Now I've changed my code to

  
    public synchronized void unregisterConnection(int record, Object data) {
        Object conn = hm.get(new Integer(record));
        if (conn != null && conn == data) {
            hm.remove(new Integer(record));
        }
        data.notifyAll();
    }

And as my code looks so simple(compared to other one's implementation), I kept wandering whether I lost something important or necessary, such as the priority between the database lock and record level lock, and so on. Any comment about that?

There is certainly nothing wrong with that approach. I used a builder pattern and kept a static protected map of LockManagers in my DataBuilder class. Actually, I think your way is more elegant. If you are using a DataFactory similar to what I did, you could even create the LockManager at the first request of a client for each Data object. You could have a method like LockManager getLockManager(Data data) in the DataFactory.

Yes, you are right. And I think the Builder pattern is more precise than the Factory pattern. Since the relationship between Data instance and LockManager, now I just add the getLockManager(Data data) in the DataFactory.

  
    /**
     * A <code>Map</code> implementaion that holds all unique <code>LockManager</code>
     * objects indexed by their relevant Data objects.
     */
    private Map lockManagerInstances = new HashMap();
    /**
     * Retrieves the <code>LockManager</code> object for the given Data instance.
     *
     * @param data         the <code>Data</code> instance for the given Data
     * @return             the <code>LockManager</code> object for this Data instance
     */
    public synchronized LockManager getLockManager(Data data) {
        LockManager lockManager = (LockManager) lockManagerInstances.get(data);
        if (lockManager == null) {
            lockManager = new LockManager();
            dataInstances.put(data, lockManager);
        }
        return lockManager;
    }
    /**
     * Closes the given database
     *
     * @param dbname the <code>String</code> path for the database file
     */
    public synchronized void close(String dbname) {
        Data db = (Data) dataInstances.get(dbname);
        if (db != null) {
            lockManagerInstances.remove(db);
            db.close();
            dataInstances.remove(dbname);
        }
    }

According to my understanding to the relationship between RemoteData, Data and LockManager, my implementation for RemoteData just as following:

  
public class DataRemoteImpl extends UnicastRemoteObject implements RemoteDataInterface {
    private DataFactory dataFactory;
    private Data data;
    private LockManager lockManager;
    public DataRemoteImpl(String dbname) throws IOException {
        dataFactory = DataFactory.getInstance();
        data = dataFactory.open(dbname);
        lockManager = dataFactory.getLockManager(data);
    }
    public DataRemoteImpl(String dbname, FieldInfo[] fields) throws IOException {
        dataFactory = DataFactory.getInstance();
        data = dataFactory.newData(dbname, fields);
        lockManager = dataFactory.getLockManager(data);
    }
}

Any comment for above code?
If above implementation are right, then comes another problem for ConnectionFactory. It's about the RemoteData constructor and database location.
As some post has mentioned:

1)Local Data - Allow user specify directory and filename of data file.
2)Remote Data - Allow user specify address of remote server and port on which it is sering the data.

If above point is right, then the ConnectionFactory(which just create RemoteData instance) should use the default path for the server side db.db, is that right? For when the client call a getConnection() method from ConnectionFactory, no path information will get, and ConnectionFactory still need to construct the RemoteData with exact dbname.
If my above understanding is not right, how about your implementation? Thanks.
Ok, having asked so many questions, there is only one left. It's about the switch between local mode and remote mode. In order to encapsulate the details for switch and make use of the DataInterface, I write some method getData(String command) to get the right instance. Any suggestions for above implementation? Anyone has the similar consideration?
So sorry for asking so many questions, but they have badgered me so much time. I just hope I can get the help from you! Thanks a lot.
-Fox

[Michael Morris]

Hi Fox,

Is that the way you mean "there was no need for Data to participate or even know there was such a thing as a LockManager"?

Yep.

I'm not so clear about your mean "verifying it actually had the lock before performing a volatile operation on Data", can you give some details for that?

My LockManager had a method:
boolean isRecordLocked(int record, Object owner)
If the object that called that method had the lock on record it returned true, otherwise false.
So a call to modify() on the remote object looked like this:

    public void modify(DataInfo newData) throws DatabaseException,
                                                  RemoteException {
        Integer record = new Integer(newData.getRecordNumber());
        if (locks.contains(record) &&
                lockManager.isRecordLocked(record.intValue(), this)) {
            db.modify(newData);
        } else {
            throw new DatabaseException("Attempt to modify unlocked record");
        }
    }

I really want to call wait() and notifyAll() on the LockManager itself, but aftering seeing so many person call the wait() and notifyAll() in the Data class, I just lost my confidence. So thanks for your explanation.

Just do it on the LockManager and keep Data out of the loop.

And as my code looks so simple(compared to other one's implementation), I kept wandering whether I lost something important or necessary, such as the priority between the database lock and record level lock, and so on. Any comment about that?

Trust me, simple is good. As far as the priority between the database lock and record lock they should be equal. I know that Mark implemented his dblock by looping thru all the records and locking each one. That obviously puts the dblock and record lock on the same priority. All you need to be concerned with is that no two clients can lock the same record concurrently and that there is no possibility of dead lock.

According to my understanding to the relationship between RemoteData, Data and LockManager, my implementation for RemoteData just as following:
...
Any comment for above code?

Looks fine.

If above point is right, then the ConnectionFactory(which just create RemoteData instance) should use the default path for the server side db.db, is that right? For when the client call a getConnection() method from ConnectionFactory, no path information will get, and ConnectionFactory still need to construct the RemoteData with exact dbname.
If my above understanding is not right, how about your implementation? Thanks.

I configured the server at boot on the command line. I had an optional parameter for the database path. If no path was entered on the command line then the server used "db.db" in the same directory as the server jar file. The path could also be selected after boot from the server GUI File menu. The client doesn't need to know where the physical location of the database file on a server.

Ok, having asked so many questions, there is only one left. It's about the switch between local mode and remote mode. In order to encapsulate the details for switch and make use of the DataInterface, I write some method getData(String command) to get the right instance. Any suggestions for above implementation? Anyone has the similar consideration?

As I mentioned earlier, I used the builder pattern to build composites for my DataInterface (mine was actually DataAccess). In the builder pattern my class the directs the building was called ConnectionDirector. According to the mode that was passed into its createConnection method was how it determined whether to build a local or remote connection object. The local connection object did not have a reference to a LockManager and the lock() and unlock() methods were empty. It also did not check the lock status before a modify(), it just modified. I'm not suggesting that you do it this way but I would like to see some more details of how you want to create the two different connections.

So sorry for asking so many questions, but they have badgered me so much time. I just hope I can get the help from you!

Please don't apologize. I love to help when I can as I am sure everyone else at javaranch does. So just keep asking your questions.
Hope this helps,
Michael Morris

[Fox Shen]

Hi Michael, thanks a lot.
You really do me a great favor.

The path could also be selected after boot from the server GUI File menu. The client doesn't need to know where the physical location of the database file on a server.

"The path could also be selected after boot from the server GUI File menu." Is this path only for the client mode?

The local connection object did not have a reference to a LockManager and the lock() and unlock() methods were empty. It also did not check the lock status before a modify(), it just modified.

Yep. My code is just similar with your idea. But now I just use Data directly for local mode. I'm not so clear about why we need such a local connection object?

I'm not suggesting that you do it this way but I would like to see some more details of how you want to create the two different connections.

Now I'm going to see some topics about Build Pattern implementation. Maybe later I can give more details in code.
Thanks,
--Fox

[Michael Morris]

Hi Fox,

"The path could also be selected after boot from the server GUI File menu." Is this path only for the client mode?

I'm not sure what you're asking here. At server startup the actual database server was not running in my implementation. There was a button on the server GUI to start the database server. Before pressing that button, the user could select a different database file from the one entered on the command line. My database server could be stopped and started any number of times as could the database file be changed.
I did not provide a means in the client to change the database file, although I did consider it. My client was also "one-shot", ie once the data connection was closed there was no way of restarting it.

Yep. My code is just similar with your idea. But now I just use Data directly for local mode. I'm not so clear about why we need such a local connection object?

Your way is fine too. You don't necessarily need a local connection object, so long as you are still making calls on the interface and not Data directly.

Now I'm going to see some topics about Build Pattern implementation. Maybe later I can give more details in code.

Don't change your overall design just to use the Builder Pattern. It sounds like you have a handle on everything already.

Hope this helps,
Michael Morris

[Fox Shen]

Thanks, Michael.

At server startup the actual database server was not running in my implementation. There was a button on the server GUI to start the database server. Before pressing that button, the user could select a different database file from the one entered on the command line. My database server could be stopped and started any number of times as could the database file be changed.
I did not provide a means in the client to change the database file, although I did consider it. My client was also "one-shot", ie once the data connection was closed there was no way of restarting it.

Now I understand what you mean. I misunderstand the Server GUI to Client GUI before.

Don't change your overall design just to use the Builder Pattern. It sounds like you have a handle on everything already.

I just want to learn more about patterns and so on. Now I just constructed some main frame, more details need to be implemented. On the way to the certification, I still have so much to learn.
-Fox

[Patrick Li]

code
-------------------------------------------------
public void modify(DataInfo newData) throws DatabaseException, RemoteException { Integer record = new Integer(newData.getRecordNumber()); if (locks.contains(record) && lockManager.isRecordLocked(record.intValue(), this)) { db.modify(newData); } else { throw new DatabaseException("Attempt to modify unlocked record"); } }
-------------------------------------------------
Michael,
I don't quite get why you want to check whether isRecordLocked(record.intValue(),this) is true or false. In the doBooking(int,int) method, we always call lock, read, modify and unlock in sequence... I assume that isRecordLocked(int, Object) will always return a true before modify(DataInfo) call and after lock(int) call, which means it is redudant(?).
Also, I noticed that the wait() call in lock(int, Object) is not in a loop(while) in some posts and Nate said wait() should be in "if". I am confused now. Ken Arnold in his book: The Java Programming Language Third Edition says that wait() should always be in a loop (page 244).
Up to now, My LockManager doesn't function like a singleton and still has a problem to lock a record. I started ten threads and each thread book a one ticket on same record ten times, so it is 100 tickets, but the total seats did not reduce 100... Somethings are wrong... I will look other posts about the testing lock...
Thanks,
Patrick

[Michael Morris]

Hi Patrick,

I don't quite get why you want to check whether isRecordLocked(record.intValue(),this) is true or false. In the doBooking(int,int) method, we always call lock, read, modify and unlock in sequence... I assume that isRecordLocked(int, Object) will always return a true before modify(DataInfo) call and after lock(int) call, which means it is redudant(?).

Call it ADP: acquired developer paranoya . Yes, we have thoroughly tested and certified our client and lock manager to be bug free and in our case the call is redundant. What happens when Fly by Night decides to expand the program to use web based clients and decides to use cheaper non-certified Java developers who introduce a tricky bug? Or perhaps it is later determined that the lockmanager is performing too poorly due to the popularity of our application, so it is decided to make the lockmanager multithreaded to improve performance and a bug is introduced there. Bugs like that are hell to track down. So by performing our check we plan for the future and may just prevent a disaster to the database. It would certainly expose the bug.

Also, I noticed that the wait() call in lock(int, Object) is not in a loop(while) in some posts and Nate said wait() should be in "if". I am confused now. Ken Arnold in his book: The Java Programming Language Third Edition says that wait() should always be in a loop (page 244).

Your right on that one. I didn't catch that in Fox's code. I'm not sure what Nate was talking about, but am relatively certain that his lock manager looped until the conditions were right for the client to acquire the lock. My locking code is wrapped in an if condition that verifies that the record number is valid, perhaps that is what Nate was talking about.

Up to now, My LockManager doesn't function like a singleton and still has a problem to lock a record. I started ten threads and each thread book a one ticket on same record ten times, so it is 100 tickets, but the total seats did not reduce 100... Somethings are wrong... I will look other posts about the testing lock...

My testing of the lock manager didn't involve any booking. I just wrote a batch file to start ten clients that randomly locked records. I also put in a provision to simulate a client crash 10% of tbe time by exiting without unlocking the previously locked record.
You should test your locking seperate from booking if it all possible, because the problem might not be locking at all.
Hope this helps,
Michael Morris

[Patrick Li]

Thanks Michael as always.
Your ADP: acquired developer paranoya really pays off.
Patrick

[Fox Shen]

Thanks, Michael and Patrick
Now to my understanding, I just added some flag called 'locked' for loop control:

  
        boolean locked = false;
        while (!locked) {
            if (record == -1) {
                dbLock = true;
                locked = true;
            }
            else {
                try {
                    if (!dbLock && !isRecordRegistered(record)) {
                        hm.put(new Integer(record), data);
                        locked = true;
                    }
                    else if (dbLock) {
                        wait();
                    }
                    else if (isRecordRegistered(record) && data!=getConnection(record)) {
                        wait();
                    }
                }
                catch (InterruptedException e) {
                }
            }
        }

Any comment?
--Fox

[Michael Morris]

Hi Fox,
You need to do something like:

    ...
    while(dblock || isRecordRegistered(record)) {
        wait();
    }
    hm.put(new Integer(record), data);
    ...

That way when the thread that gets the ball discovers that his record is still locked by another client goes back in the wait pool. Otherwise, the record is put in the hashmap and the thread leaves the method to finish the booking operation.
Hope this clears it up,
Michael Morris

[christy smile]

Hi, Michael,
I remember that you mentioned in the post that you keep a hashtable in the remoteDataAccess to keep track of the records being locked by that client (so that the locked records can be cleaned up later in the unreferenced method). I would like to know is hashtable is neccessary, would a Vector do this trick?
Thank you!
Christy

[Michael Morris]

Hi christy,
Actually I kept a HashSet of locked records, nothing to map them to. You really should try to avoid using Vector and the older data structures when possible. An ArrayList is very similar to a Vector and is in the new Collections API, which makes it preferrable to a Vector.
Hope this helps,
Michael Morris

[Chandra Peri]

Hi Michael,

<Michael Morris>
Actually I kept a HashSet of locked records, nothing to map them to.
</Michael Morris>

The instructions I've got, says that If an attempt is made to unlock a record that has not been locked by this connection, then no action is be taken. So I find that we've got map the connection with the records locked by the connection somewhere. In my case I'm currently using Hashtable, and it's in early stages of dev, so I may change it, when I start my cleanup/organize/optimize work. Thoughts!
(Become a member today! my warm regards to senior members)

[Michael Morris]

Hi Chandrakumar,
I did map them in the LockManager to the objects that requested the locks. What I meant here was that in the remote connection object I kept a set of locked records so that if the client connection died when unreferenced() was called by the RMI runtime, I could iterate thru the set and release all the locks.
Michael Morris

[Fox Shen]

Thanks, Michael.
But I still cannot quite understand the change you provided:

while (dblock || isRecordRegistered(record)) {
wait();
}
hm.put(new Integer(record), data);

Since isRecordRegistered() method will return true when the record is registered no matter whether it is locked by current connection or others. So if the record has been locked by current connection, why should it still wait in the pool?
Is there any false with my previous code for registerConnection() method? Is it contains some structure error? Pls comment.
Thanks,
-- Fox

[Michael Morris]

Hi Fox,
Here is your registerConnection() which I assume is the equivalent of my lock() method:

public synchronized void registerConnection(int record, Object data) {
    if (record == -1) {
        dbLock = true;
    }
    if (!dbLock && !isConnectionRegistered(record)) {
        hm.put(new Integer(record), data);        
    } else {
        try {
            data.wait();
        } catch (InterruptedException e) {}
    }
}

Follow this scenario using your method above:
Thread A calls registerConnection(5, this) and acquires the lock on record 5. Next thread B calls registerConnection(6, this) and acquires the lock on record 6. Then thread C calls registerConnection(5, this) but since A already has that lock goes into the wait pool. Now thread D calls registerConnection(6, this) and for the same reason has to wait. Finally thread A calls unregisterConnection() on record 5 and at the notifyAll() call by random chance thread D comes out of the pool expecting to lock record 6 which is still owned by thread B. By your code, thread D completes the method. Now does thread D believe he owns the lock to record 6?
What I am saying is, you should loop until you know the record that you want is available to lock. If you have 20 clients, 10 vying for the same record, you have no idea which client is going to get the chance to execute when notifyAll() is called. You have to check the record each time the client comes out of the wait pool and if the record is still unavailable, call wait() again.
Hope this clears it up,
Michael Morris

[Chandra Peri]

Thanks Michael!

[Fox Shen]

Hi Michael,
Thanks for your details reply. Now I know what's wrong with my code.
-- Fox

[Patrick Li]

Hi Michael,
I hit a big bump. My lock implementation behaves like database lock, not a record lock. At any moment, the size of HashMap in LockManager is either 1 or 0, never exceeds 1 though a lot of threads tried to book different flights. The doBooking(int,int)[including lock,read,modify and unlock] is synchronize, otherwise, it hanged and deadlocked....
what is my problem? could you give me some hints?
Thanks,
Patrick

[parthiban subramaniam]

Hi guys,
all that says that i can remove the lock and unlock method out of the data class and provide it only in my remote interface and remote impl ... am i assuming right or making a ass out of myself here
cheers,
parthi

[Michael Morris]

Hi Parthi,

all that says that i can remove the lock and unlock method out of the data class and provide it only in my remote interface and remote impl ... am i assuming right or making a ass out of myself here

Well I can't say whether you are making an ass out of yourself here since we're not drinking tequilla and you're not picking a fight with the biggest guy in the bar, but you've got everything else right.

Michael Morris

[Michael Morris]

Hi Patrick,

I hit a big bump. My lock implementation behaves like database lock, not a record lock. At any moment, the size of HashMap in LockManager is either 1 or 0, never exceeds 1 though a lot of threads tried to book different flights. The doBooking(int,int)[including lock,read,modify and unlock] is synchronize, otherwise, it hanged and deadlocked....
what is my problem? could you give me some hints?

What is the condition imposed for locking a record? Here is the first few lines of my lock() method:

        if (((record <= db.getRecordCount()) && (record > 0)) ||
                                                 record == DBLOCK) {
            Integer recordNumber = new Integer(record);
            try {
                while(dbLocked || lockedRecords.containsKey(recordNumber) ||
                        ((record == DBLOCK) && !lockedRecords.isEmpty())) {
                    wait();
                }
            } catch (InterruptedException ignore) {}

That's the first place you need to look to see why you can't lock an ordinary record and can lock the database. As for dead locking, in your unlock method, the call to notifyAll() should be unconditional, ie don't put it in an if block. Make sure that notifAll is called every time that unlock() is called.
Hope this helps,
Michael Morris

[Ronnie Phelps]

I've learned alot from this post but there's one thing that I don't understand. What is the purpose of locking on the entire Database? Maybe this requirement is not in the bet exam, but I can't seem to understand why or when this should be done.

[Sai Prasad]

For database mainenance you want to prevent from clients modifying data in the tables. Therefore you want to use the LockManager to put all the client requests in a queue until the database is ready.