Locking/Unlocking, -- Am I Done Or I Don't Understand Requirements?

[John Smith]

There is a huge number of threads on locking/unlocking, and I hate to add another one, but I just don't understand what is all the noise about.
I needed exactly 17 lines of code to implemented it (code below). I tested it with 50 threads each invoking the database "write" 10 times in a row to increase the "Price" field of the same record by $1 (all threads working in networked mode). No exceptions were generated, and the field value increased by $500, as expected. And yes, the lock() and unlock() are called by the client.
I understand that my code doesn't have a safeguard against a client that "forgets" to unlock the record, but do we *really* need to implement that, according to our assignment instructions? If not, is that all I had to do for locking/unlocking?
Thanks,
Eugene Kononov.

public class RecordLockManager {
    private HashSet locks = new HashSet();
    public void lock(int recNum) throws InterruptedException {
        synchronized(locks) {
            Integer record = new Integer(recNum);
            while (locks.contains(record))
                locks.wait();
                locks.add(record);
            }
    }
    public void unlock(int recNum) {
        synchronized(locks) {
            locks.remove(new Integer(recNum));
            locks.notifyAll();
        }
    }
}

[John Smith]

Sorry, the indentation was screwed in the code, here it is again:

public class RecordLockManager {
    private HashSet locks = new HashSet();
    public void lock(int recNum) throws InterruptedException {
        synchronized(locks) {
            Integer record = new Integer(recNum);
            while (locks.contains(record))
                locks.wait();
            locks.add(record);
        }
    }
    public void unlock(int recNum) {
        synchronized(locks) {
            locks.remove(new Integer(recNum));
            locks.notifyAll();
        }
    }
}

[Peter den Haan]

You don't need the synchronized blocks -- just use synchronized methods, they are easier to grok.
In my copy of the assignment (> 2 years ago), the javadoc for unlock() had a statement to the effect that a call to unlock for a record no longer owned by the client should be ignored. This cannot be done without tracking lock ownership in some fashion. I also figured that for consistency's sake this meant that a duplicate call to lock() should be ignored as well.
- Peter

[John Smith]

Peter den Haan posted:

In my copy of the assignment (> 2 years ago), the javadoc for unlock() had a statement to the effect that a call to unlock for a record no longer owned by the client should be ignored.

My assignment says something similar:

If an attempt is made to unlock a record that has not been locked by this connection, then no action is to be taken.

But the way I interpreted it is this:
If the record is not locked (not in the collection), ignore the call to unlock (do not remove anything from collection).
So, do we or do we not need to implement client tracking?
Thanks,
Eugene Kononov.

[Branko Paskutini]

Eugene,
Yes you need to keep track on who has a lock on specific record. If user A has a lock on record 12, only user A can call an unlock on that record. If user B calls unlock on record 12, it would be ignored. So you need to find a way to track each user. In other words, each user connecting to the Server needs to be uniquely identified.

[Peter den Haan]

Originally posted by Eugene Kononov:
But the way I interpreted it is this:
If the record is not locked (not in the collection), ignore the call to unlock (do not remove anything from collection).

Ah, but beware of race conditions!

Client A calls unlock(123).

Client B calls lock(123).

Client A calls unlock(123) again.

Now what?
- Peter
[ May 12, 2002: Message edited by: Peter den Haan ]

[John Smith]

Peter den Haan posted:
You don't need the synchronized blocks -- just use synchronized methods, they are easier to grok.

Peter,
I am not sure what you mean, -- I need to synchronize on *locks*. If I synchronize the method, it will synchronize on *this*, which is the LockManager. It will not work. Can you elaborate pls?
Thanks,
Eugene Kononov.

[Peter den Haan]

LockManager instances and "locks" have a 1:1 relationship. Functionally, it doesn't matter on which you synchronize -- but synchronizing on the lock manager makes your code easier to read.
- Peter

[dean tomlinson]

hi peter / eugene

quote:
--------------------------------------------------------------------------------
Originally posted by Eugene Kononov:
But the way I interpreted it is this:
If the record is not locked (not in the collection), ignore the call to unlock (do not remove anything from collection).
--------------------------------------------------------------------------------
Ah, but beware of race conditions!
Client A calls unlock(123).
Client B calls lock(123).
Client A calls unlock(123) again.
Now what?

i cannot see any problem with the scenario above. which makes me think i've over looked something. please correct me...
when client A calls unlock(123), it is removed from it personal lockedRecords collection, and then the servers lockRecords collection.
when client B calls lock(123) it checks to see that 123 is not already locked, and as it has just been unlocked by client A it is able to obtain the lock.
when client B calls unlock(123) again, it checks to see if the recrod number 123 is in it's personal lockedRecords collection. as it is not, the method does nothing else and returns.
where does the race condition come into this ?

[Peter den Haan]

Dean, client A makes the last (redundant) call to unlock(). I assume that's what you meant. In any case, you mention a personal lock collection that the client has -- which is effectively one of the possible ways to track lock owners.
- Peter
[ May 14, 2002: Message edited by: Peter den Haan ]

[Ramesh kumaar]

Hello All,
Here in the bellow code i think its not adviseable to use notifyAll(), Instred u can use notify(). Correct me if am wrong.
regards,
-rameshkumar
public void unlock(int recNum) {
synchronized(locks) {
locks.remove(new Integer(recNum));
locks.notifyAll();
}
}

[Ramesh kumaar]

Hello All,
Here in the bellow coding what is the need for while loop, We can very well use if condition. I feel its wastage of processer.
-rameshkumar

public void lock(int recNum) throws InterruptedException {
synchronized(locks) {
Integer record = new Integer(recNum);
while (locks.contains(record))
locks.wait();
locks.add(record);
}
}

[Peter den Haan]

Originally posted by Ramesh kumaar:
Here in the bellow code i think its not adviseable to use notifyAll(), Instred u can use notify(). Correct me if am wrong.

Consider yourself corrected
If you were to use notify(), the thread being woken up may not wait for the specific lock that has just become available; the thread that does wait for the lock wil not get it. As a general point, notify() is useful only if all waiting threads are interchangeable.
- Peter
[ May 15, 2002: Message edited by: Peter den Haan ]

[Matthew Comer]

Yes, Peter has it right. These threads are NOT interchangeable, as each one could be waiting on a DIFFERENT lock.
However, you could design a lock manager that used seperate semaphores for each lock. This would allow you to wake up only the threads that were waiting for the lock that was being released. This might be more effecient because it would prevent irrelevant threads from being activated only to put themselves back to sleep. However this would be more complicated, and would require more object creation and more memory use. Wonder if anyone has implemented such a lock manager for the developer assignment?

[Peter den Haan]

Personally I considered it, but rejected it as too complicated given a general expectation that lock contention will be limited -- there will be few threads waiting for a lock.
- Peter

[John Smith]

Peter den Haan wrote:

LockManager instances and "locks" have a 1:1 relationship. Functionally, it doesn't matter on which you synchronize -- but synchronizing on the lock manager makes your code easier to read.

Peter, what if I want to use my LockManager as a pure service class, with all its methods static, as in:

public class RecordLockManager {
    private static Map locks = Collections.synchronizedMap(new HashMap());
    public static void lock(int recNum, Object client) throws InterruptedException {
        synchronized(locks) {
            Integer record = new Integer(recNum);
            while (locks.containsKey(record))
                locks.wait();
            locks.put(record, client);
            System.out.println("client " + client + " locked record " + recNum);
        }
    }
    public static void unlock(int recNum, Object client) {
        synchronized(locks) {
            Integer record = new Integer(recNum);
            if (locks.containsKey(record) && locks.get(record) == client) {
                locks.remove(new Integer(recNum));
                System.out.println("client " + client + " unlocked record " + recNum);
            } else {
                System.out.println("****** client " + client + " tried to unlock record " + recNum);
            }
            locks.notifyAll();
        }
    }
}

Now I *have* to synchronize on locks. It seems to me that this is what LockManager should be, -- static methods that manipulate static collection containing locks. Using your terminology, this represents the server:lockmanager:locks relationship as 1:1:1. What do you think?
Thanks,
Eugene Kononov.

[Peter den Haan]

Originally posted by Eugene Kononov:
Peter, what if I want to use my LockManager as a pure service class, with all its methods static [...] I *have* to synchronize on locks.

Two things.
First, the comment still stands. You would have a class-scoped (static) lock map, accessed by static methods, and functionally it doesn't make a difference whether you synchronize on the Map or on the RecordLockManager.class object (which is what synchronized methods would do).
Second, this would not be a good idea. How many databases have you ever used that had just one table? -- at some point in the future, you will want to use multiple RecordLockManagers. When the whole thing is static, you're stuffed. But even if all you ever needed was a single instance, there are still problems with this approach from a Java-OO point of view: as static methods are bound early (compile-time), it would be hard to create a subclass and plug that in somewhere instead of its parent.
- Peter
[ May 16, 2002: Message edited by: Peter den Haan ]

[John Smith]

How many databases have you ever used that had just one table? -- at some point in the future, you will want to use multiple RecordLockManagers.

Peter,
You really got me thinking now, thanks.
Eugene Kononov.

[John Smith]

After much of refactoring, I think I finally got it right. For every new client, the connection factory returns a new remote object, and passes the references to instance of Data and LockManager as arguments, such as in:

public class ConnectionFactoryImpl extends UnicastRemoteObject implements ConnectionFactory {
    private Data data;
    private RecordLockManager lockManager;
    ...
    synchronized public DBServer getConnection() throws RemoteException {
        return new DBServerImpl(data, lockManager);
    }
}

This remote object that the client gets (DBServerImpl in my case) implements the Data methods by delegating the calls to Data methods.
One little thing that I can't figure out is how do I make it so that the lock()/unlock() methods are only called when the client runs in the remote mode, without writing multiple ifs?
My clent class (I call it FBNService) that uses the Data has two constructors, 1 for local mode, 1 for remote mode. It comes down to:
Local mode:
data = new Data(dbName);
Remote mode:
data = connectionFactory.getConnection();
Now, if I have to define method bookFlight() in my FBNService class, it will look something like:

public void bookFlight() {
...
data.lock(record);
data.getRecord();
data.modify(record);
data.unlock(record);
...
}

The problem is, I don't want to call lock/unlock if it is a local mode. What would be a good way to resolve it?
Thanks,
Eugene Kononov.

[Sai Prasad]

Eugene,
Looks good. I have couple of comments.
1) Instead of keeping a private instance of Data in your ConnectionFactory, create another Factory called DataFactory and pass the database file name to the DataFactory to get the Data instance.
Then you can pass the Data instance to the Connection object. You can use the DataFactory in the local mode also. This way, your ConnectionFactory can handle more than one tables.(Extensibility!)
2) It is ok to lock/unlock in the local mode. I would justify it saying "Data class is flexible to handle both local and remote access!"
[ May 16, 2002: Message edited by: Sai Prasad ]

[John Smith]

Sai Prasad posted:

1) Instead of keeping a private instance of Data in your ConnectionFactory, create another Factory called DataFactory and pass the database file name to the DataFactory to get the Data instance.
Then you can pass the Data instance to the Connection object.

So then the client will do something like:
data = connectionFactory.getConnection("db.db");
or even
data = connectionFactory.getConnection(FLIGHTS_TABLE);
data = connectionFactory.getConnection(SOME_OTHER_TABLE);
Right?

It is ok to lock/unlock in the local mode.

After thinking about it, now I realize that perhaps we should lock/unlock in local mode, because the client implementation may change to run some batch reservation job from multiple threads. So even with local database, it makes sense to lock? Then where do you do the locking to accomodate both local and remote locking?
Thanks,
Eugene Kononov.

[Sai Prasad]

I wouldn't pass the database file name from the client to the RMI server. I would pass the database file name as the configuration parameter and read it during the ConnectionFatory startup.

[Ramesh kumaar]

Hi Peter,
Originally Posted by peter
--------------------------------------------------
Dean, client A makes the last (redundant) call to unlock(). I assume that's what you meant. In any case, you mention a personal lock
collection that the client has -- which is effectively one of the possible ways to track lock owners.
--------------------------------------------------
I feel the condition u mentioned above will not occur according to the requirement of Sun, Here following is the sequence of method call while booking a ticket.
lock---->readRec--->modify--->unlock
So where comes the reduntent call of unLock() method.
-rameshkumar
Since the client program always

[Ramesh kumaar]

Hi Peter,
--------------------------------------------------
public class RecordLockManager {
private HashSet locks = new HashSet();
public void lock(int recNum) throws InterruptedException {
synchronized(locks) {
Integer record = new Integer(recNum);
while (locks.contains(record))
locks.wait();
locks.add(record);
}
}
public void unlock(int recNum) {
synchronized(locks) {
locks.remove(new Integer(recNum));
locks.notifyAll();
}
}
}

In my copy of the assignment (> 2 years ago), the javadoc for unlock() had a statement to the effect that a call to unlock for a record no
longer owned by the client should be ignored. This cannot be done without tracking lock ownership in some fashion. I also figured that
for consistency's sake this meant that a duplicate call to lock() should be ignored as well.
- Peter
--------------------------------------------------
I case of the above code
while (locks.contains(record))
locks.wait();
locks.add(record);
Once a client-A calls a lock this checks for existency if no one owns the lock for the recordno then Client-A owns the record by ADDING the recordno to the HashSet
Now after owning the lock client-A books the ticket and calls the unLock(). Here according to the coding above every client has to remove the recordNo from the dataStructure(HashSet), Wheather
the Client owns are not. Please keep in mind the sequence of booking is lock--->read--->modify--->unlock.
Please correct me if iam wrong.
regards
-rameshkumar

[Ramesh kumaar]

Hi Peter,
--------------------------------------------------
public class RecordLockManager {
private HashSet locks = new HashSet();
public void lock(int recNum) throws InterruptedException {
synchronized(locks) {
Integer record = new Integer(recNum);
while (locks.contains(record))
locks.wait();
locks.add(record);
}
}
public void unlock(int recNum) {
synchronized(locks) {
locks.remove(new Integer(recNum));
locks.notifyAll();
}
}
}

In my copy of the assignment (> 2 years ago), the javadoc for unlock() had a statement to the effect that a call to unlock for a record no
longer owned by the client should be ignored. This cannot be done without tracking lock ownership in some fashion. I also figured that
for consistency's sake this meant that a duplicate call to lock() should be ignored as well.
- Peter
--------------------------------------------------
I case of the above code
while (locks.contains(record))
locks.wait();
locks.add(record);
Once a client-A calls a lock this checks for existency if no one owns the lock for the recordno then Client-A owns the record by ADDING the recordno to the HashSet
Now after owning the lock client-A books the ticket and calls the unLock(). Here according to the coding above every client has to remove the recordNo from the dataStructure(HashSet), Wheather
the Client owns are not. Please keep in mind the sequence of booking is lock--->read--->modify--->unlock.
Please correct me if iam wrong.
regards
-rameshkumar

[Ramesh kumaar]

Hello Peter,
Iam waiting for ur reply.
-rameshkumar

[John Smith]

Ramesh posted:

Hello Peter, Iam waiting for ur reply.

Peter took a vacation to visit Dubai, Mumbai, and Kubai.
I thing the answer to your question is that a client that didn't call lock() may call unlock() on that record and then the integrity of data is in question.
Eugene.

[Ramesh kumaar]

Hi Eugene,
--------------------------------------------------
I thing the answer to your question is that a client that didn't call lock() may call unlock() on that record and then the integrity of data is
in question.
Eugene.
--------------------------------------------------
How can this happen,According to sun's requirement
If a client wants to book a ticket first he should own the lock by calling lock method which intern checks for existency of the same recordNo. If so waits else Owns the lock by adding the recNo to a dataStructure (HashSet). then readRec,modifyRec,unLock. Here Where comes the need to call unLock() with out calling lock() method. Hope understand the problem.

-rameshkumar

[John Smith]

Ramesh wrote:

If a client wants to book a ticket first he should own the lock by calling lock method...

Yes, but a sloppy (or malicious) client might also call unlock() on the record that it didn't lock.
Eugene.