Why I lost points in locking, pls comment on my code

[Zee Ho]

Hi, ranchers, I got 354/400 and lost 36 points in locking like most of the people who passed without a high scores.following is my implementation:
LockManager
----------------------------------

public long lockRecord(final long recNo) {
    long cookie = //get the cookie as identifier
    try {
        //cookies is the hashmap to store the locked record.
        synchronized (cookies) {
            while (cookies.containsKey(new Long(recNo))
                    || cookies.containsKey(new Long(LOCK_DB))) {
                cookies.wait();
            }
            cookies.put(new Long(recNo), new Long(cookie));
        }
    } catch (InterruptedException e) {
              ////// 
    }
    return cookie;
}

1. I do synchronize on every method which may move the file pointer
like find, insert and delete.
2. the procedure of update/insert/delete is
-lockRecord
-check the record exist or not.
-doAction
-unlock
3. I make my RemoteData implements the Unreferenced to clean the lock.

Is it the correct track of implementing the lock mechanism? I can not figure out why I lost 36 in this aspect. can any one comment on my way? thx very much..

[Andrew: Removed unLockRecord method. For reasons why, see "What is the policy on posting questions I saw on the exam / details of how to do the assignment?" in the JavaRanch SCJD FAQ
[ June 18, 2005: Message edited by: Andrew Monkhouse ]

[Andrew Monkhouse]

Hi Zee,

Personally I would not have created a new Long for LOCK_DB on each iteration of the while loop - I would have had a constant for it (even a private constant). Likewise I would not have created a new Long for recNo on each iteration - you could create a Long outside the synchronized block (and a Long for the cookie value itself). These are fairly minor points though.

The two things that I think are more important are:

What happens if there are 2 or more threads waiting for the record, and the record is deleted? You don't seem to do any validation on whether a record has been deleted or not.

What happens in your InterruptedException block?

Regards, Andrew

[Andrew Monkhouse]

Congratulations on passing

[John Smith]

In additions to Andrew's comments, your "lock entire database" methodology is a bit rough -- it locks the entire database even if there are locked records. In my implementation, I waited until locks are empty before locking the database. However, it's hard to say if it was one of the reasons that they deducted points from you. What do your requirements say about locking the entire database?

[Zee Ho]

thx for the replies
1.For the validation Andrew metioned, actually, what I did is: For any thread I will let it check the record exist or not after it locked the desired record. If the record is already deleted, it will throw the RecordNotFoundException, else It can do whatever it should do.
-lockRecord
-check the record exist or not.
-doAction.
-unlock.

2. For the InteruptException, I just simply log the exception with java logging.

3. yes. I realized that my implementation is not that perfect in locking entire DB, but in my requirement, they do not mentioned anything about it, that is just part of my design.
[ June 18, 2005: Message edited by: Zee Ho ]

[John Smith]

What method did you use to generate cookies?

[Zee Ho]

I use the most simple way

1. I define a cookie like:
private static long clientId = 1;

And I developed a method named getClientId(it marks as synchronized), in the method I just simply return clientId++

[Zee Ho]

BTW, my LockManager is singleton

[Lara McCarver]

The problem with the way you are catching InterruptedException is that it is outside the while loop. If your thread is interrupted, when it resumes, it is not going to test the while() loop anymore, it is just going to exit.

Another concern I have is, you are waiting on the cookies object. I am assuming that this object is notified when any of the records is unlocked. I realize my lock method is a little different because I don't use cookies, but what it says is:

// Locks a record so that it can only be updated or deleted by this client.
// If the specified record is already locked, the current thread gives up
// the CPU and consumes no CPU cycles until the record is unlocked.
public void lock(int recNo) throws RecordNotFoundException;

So if I am understanding this correctly, if the user is locking record 5, you are going to be notified when record 14 is unlocked. So if your lock method has the same type of requirement (a big IF), then I would say that this doesn't satisfy the requirement, although I have heard other people say that it is OK.

[Zee Ho]

I do not agree with the part I you mentioned, I think there is nothing wrong, such code snippet means:

If Thread A(TA) got the lock and then Tread B (TB) try to update the record with same index, our lock manager forces the TB to keep in waiting, however, some exception happens WHILE TB waiting for the lock. in this scenario, there is nothing wrong to stop the loop, log the error and exit(abort from the current execution(eg, update), not shutdown the server) is the proper behavior.

For the part II, I think what you said is reasonable, I never consider that before. I need some enhancement in this part, But I do not think it worth 36 points.

Please rectify me If I am still in confusion

[Alan Morgan]

public long lockRecord(final long recNo) {
    long cookie = //get the cookie as identifier
    try {
        //cookies is the hashmap to store the locked record.
        synchronized (cookies) {
            while (cookies.containsKey(new Long(recNo))
                    || cookies.containsKey(new Long(LOCK_DB))) {
                cookies.wait();
            }
            cookies.put(new Long(recNo), new Long(cookie));
        }
    } catch (InterruptedException e) {
              ////// 
    }
    return cookie;
}

Following on from the points Lara made:
1. I have my wait() in a try catch(InterruptedException) so after interrupt the condition is checked again.
2. I have a notifyAll() before I exit method so any waiting thread is notified.

Question
---------
Why would you want to lock the whole DB ?...I don't see a requirement for this in my project reqs but I also haven't heard it mentioned on here before (although I haven't really been looking to be honest)


Thanks,
Alan.


PS Congrats on passing

[ June 20, 2005: Message edited by: Alan Morgan ]

[Lara McCarver]

I have created a Vector of RecordLock, with one RecordLock per database record. Each record lock keeps track of whether it is locked and if so, which object locked it.

I initially had a notifyAll() at the end of my lock() method, just before I exited, just like you. However, I eventually realized that locking a record did not make another waiting record eligible to use it, and in addition, the requirement

// If the specified record is already locked, the current thread gives up
// the CPU and consumes no CPU cycles until the record is unlocked

does explicitly say to only use the CPU when the record is unlocked. so I changed to only do a notifyAll() on the unlock() method.
[ June 20, 2005: Message edited by: Lara McCarver ]

[Alan Morgan]

Originally posted by Lara McCarver:
I have created a Vector of RecordLock, with one RecordLock per database record. Each record lock keeps track of whether it is locked and if so, which object locked it.

I initially had a notifyAll() at the end of my lock() method, just before I exited, just like you. However, I eventually realized that locking a record did not make another waiting record eligible to use it, and in addition, the requirement



does explicitly say to only use the CPU when the record is unlocked. so I changed to only do a notifyAll() on the unlock() method.

[ June 20, 2005: Message edited by: Lara McCarver ]

In the words of one Homer Simpson....doh

Of course you are correct and it makes no sense to notifyAll in the lock().
I'd like to think I would have caught that during my review but then again....

Alan.

[Ta Ri Ki Sun]

Originally posted by Alan Morgan:



In the words of one Homer Simpson....doh

Of course you are correct and it makes no sense to notifyAll in the lock().
I'd like to think I would have caught that during my review but then again....

Alan.

Ouch, also missed that. The only thread waiting for notification is one hoping to get a lock, so unlock is the only method that needs notify/notifyAll.

Thanks Lara

[Zee Ho]

yes, Agree with your guys. the unlock method is the only place I invoked the notifyAll()

So, I still confuse about why I lost point. seems there is no big mistake I made.

[David Sham]

public long lockRecord(final long recNo) { long cookie = //get the cookie as identifier try { //cookies is the hashmap to store the locked record. synchronized (cookies) { while (cookies.containsKey(new Long(recNo)) || cookies.containsKey(new Long(LOCK_DB))) { cookies.wait(); } cookies.put(new Long(recNo), new Long(cookie)); } } catch (InterruptedException e) { ////// } return cookie;}

I just received my exam scores and somehow got a perfect score on the locking mechanism for my project(am still surprised ). So Zee asked for my 2 cents.

From the code above, the only thing I can offer is that the synchronization occurs on a Hashtable object within the method. It is also within a try/catch block which catches an InterruptedException. From a maintainability standpoint, somebody could add additional code within the try/catch, but outside the synchronized block. This added code may not pertain to the enclosing try/catch block which would mean the try/catch block would be encompassing unrelated code. This could become difficult to maintain.

Another approach would be to just synchronize the entire method and only put in the method what needs to be synchronized. I did this in my solution. This way, I'm not calling wait() on the synchronized object, but on the executing thread intstead. My unlock method was also synchronized, and I just called notifyAll() on the executing thread there as well.

Now, there are really 2 parts to the locking mechanism that must be accounted for. One is tracking the holding client (the client holding the locked record) with the lock cookie used (assuming you are using lock cookies in your assignment). The other is tracking the lock cookie to the locked record. This is to ensure that the holding client accesses the correct record when performing an operation that requires locking. I used 2 Hashtables: 1 to track the holding client ID (a unique Integer value) and the lock cookie, and the other to hold the lock cookie and record number.

My solution used a timeout option in which the locking manager forced an unlock if a lock on a record timed out (after 30 seconds). This prevents thread deadlock from occuring.

Otherwise, who knows why people get the scores they do. I got a perfect score for locking, but got only 7/40 for network server .

Congratulations Zee on passing the certification. Don't let your locking score get to you. The important thing is that you're now an SCJD!!

[Andrew Monkhouse]

Alan & Zee,

The code posted shows the while loop being inside the try...catch block (as Lara mentioned). This means that if the lock thread is interrupted at all, then you will automatically jump out of the while loop.

Code example. Here is a simple server as described above:

import java.io.*;
import java.rmi.*;
import java.rmi.registry.*;
import java.rmi.server.*;
 
interface ServerApplication extends Remote {
    public int doSomething() throws RemoteException;
}
 
public class Server extends UnicastRemoteObject implements ServerApplication {
    private static Data data = new Data();
 
    public Server() throws RemoteException {
        // do nothing constructor
    }
 
    public static void main(String[] args) throws Exception {
        LocateRegistry.createRegistry(1099);
        Naming.rebind("Server", new Server());
    }
 
    public int doSomething() throws RemoteException {
        return data.lock();
    }
}
 
class Data {
    public int lock() {
        int cookie = (int) (Math.random() * 100);
        try {
            synchronized (Server.class) {
                for ( ;; )  {
                    System.out.println("Server waiting forever");
                    Server.class.wait();
                }
            }
        } catch (InterruptedException ie) {
            System.out.println("Received interrupted exception");
        }
        return cookie;
    }
}

And the client code that goes with it:

import java.rmi.*;
import java.rmi.registry.*;
 
public class BadClient extends Thread {
    public static void main(String[] args) throws Exception {
        ServerApplication server = (ServerApplication) Naming.lookup("Server");
        System.out.println("cookie = " + server.doSomething());
    }
}

Everything likes fine and good, and it seems to run OK.

But then the programmers writing the client programs complain that they need to have a response from the lock method within 5 seconds. When looking at this request, the team leader says that nobody is allowed to touch the Data class since it is working. So the junior programmer decides to change the doSomething method on the server, like so:

    public int doSomething() throws RemoteException {
        final Thread lockingThread = Thread.currentThread();
        Thread monitorThread = new Thread() {
            public void run() {
                try {
                    Thread.sleep(5000);
                    System.out.println("About to give lock method a kick");
                    lockingThread.interrupt();
                } catch (Exception e) {
                    e.printStackTrace();
                    System.exit(-1);
                }
            }
        };
        monitorThread.start();
        return data.lock();
    }

Now when we run it, we see on the server side:



And on the client side:



From the client's perspective, I just received a cookie without ever getting the lock!

Yes, I know that you could claim that the junior programmer did the wrong thing in how they implemented the interrupt (and, more importantly, how they didn't check whether the thread was interrupted or not) - but in reality the situation is that the Data class' lock method should not have returned a cookie in this situation!

As for what should you do in this situation ... I have written enough for a while - time for someone else to put forward some thoughts .

Regards, Andrew
[ June 20, 2005: Message edited by: Andrew Monkhouse ]

[Mihai Radulescu]

Hi Andrew,

What you mean by "implemented the interrupt" ?
In my locking manager I throw an IntException if a thread which is waiting is interrupted.
So in the other words I have a normal/default behavior and for extreme cases (interrupted cases) i throw exceptions.
Am I right ?

[Alan Morgan]

Andrew,

When I said:

1. I have my wait() in a try catch(InterruptedException) so after interrupt the condition is checked again.


I wasn't 100% clear. What I meant is that my wait is in a try catch and the try catch is inside the while as so:

while (lockMap.containsKey(new Integer(recNo)))
{
try
{
lockMap.wait();
}
catch (InterruptedException inte){}
}



Will I still suffer from the problem you outlined ?

Tks,
Alan.
[ June 21, 2005: Message edited by: Alan Morgan ]

[Alan Morgan]

Originally posted by David Sham:



I just received my exam scores and somehow got a perfect score on the locking mechanism for my project(am still surprised ). So Zee asked for my 2 cents.

From the code above, the only thing I can offer is that the synchronization occurs on a Hashtable object within the method. It is also within a try/catch block which catches an InterruptedException. From a maintainability standpoint, somebody could add additional code within the try/catch, but outside the synchronized block. This added code may not pertain to the enclosing try/catch block which would mean the try/catch block would be encompassing unrelated code. This could become difficult to maintain.

Another approach would be to just synchronize the entire method and only put in the method what needs to be synchronized. I did this in my solution. This way, I'm not calling wait() on the synchronized object, but on the executing thread intstead. My unlock method was also synchronized, and I just called notifyAll() on the executing thread there as well.

Now, there are really 2 parts to the locking mechanism that must be accounted for. One is tracking the holding client (the client holding the locked record) with the lock cookie used (assuming you are using lock cookies in your assignment). The other is tracking the lock cookie to the locked record. This is to ensure that the holding client accesses the correct record when performing an operation that requires locking. I used 2 Hashtables: 1 to track the holding client ID (a unique Integer value) and the lock cookie, and the other to hold the lock cookie and record number.

My solution used a timeout option in which the locking manager forced an unlock if a lock on a record timed out (after 30 seconds). This prevents thread deadlock from occuring.

Otherwise, who knows why people get the scores they do. I got a perfect score for locking, but got only 7/40 for network server .

Congratulations Zee on passing the certification. Don't let your locking score get to you. The important thing is that you're now an SCJD!!

Ok David,

Its stupid question time again....

1. Why lock the whole DB ? (reference to LOCK_DB)

2. You say you keep a hashtable of Client ID and lockCookie to ensure that the holding client accesses the correct record when performing an operation that requires locking.

I'm a bit confused.
When I successfully lock a record I get back a lockCookie.
Now any methods that change the data must pass in a recordNum and the lockCookie. If they don't match then you can't do it.

I can't see why you need the second hashtable ?



Cheers,
Alan.

[Zee Ho]

1. Why lock the whole DB ? (reference to LOCK_DB)

This will be useful when you insert the new record to the database, I think most of the candidate use the physical index of record as the record identifier, consider about this:

The record 01, 02, 03 exist in the database, the user are trying to create a new record, in the sametime, someone delete the 02, but you may get 04 as your new index (increase 03 as your new index). In some specification, Sun requires developers to reclaim the space of the deleted record, considering such requirement, If the user is able to lock whole DB, then he can get idea about whether there are some deleted record, if so, he can pick up the 02 and reuse the same space.

2. You say you keep a hashtable of Client ID and lockCookie to ensure that the holding client accesses the correct record when performing an operation that requires locking.

Most of the candidate use the 1 hashmap(hashtable) solution, it is enough, since the lockCookie itself can identify whether you are the one who locks the record or not.
[ June 21, 2005: Message edited by: Zee Ho ]

[Zee Ho]

Andrew, Lara. Now I got what u mean now, yes, it is something I did not consider about.

Alan I think you are in correct track.
[ June 21, 2005: Message edited by: Zee Ho ]

[David Sham]

2. You say you keep a hashtable of Client ID and lockCookie to ensure that the holding client accesses the correct record when performing an operation that requires locking.

I'm a bit confused.
When I successfully lock a record I get back a lockCookie.
Now any methods that change the data must pass in a recordNum and the lockCookie. If they don't match then you can't do it.

I can't see why you need the second hashtable ?

I used the following criteria for my locking solution:

Hastable 1: holdingClientID, lockCookie
If the passed in lockCookie does not equal the lockCookie for the passed in holdingClientID, then throw a SecurityException. This ensures that the correct client is attempting to access the desired record no with the correct lockCookie.

Hashtable 2: lockCookie, recordNo
If the passed in recordNo does not equals the recordNo for the passed in lockCookie, then throw a SecurityException. This ensures that the correct lockCookie is used to access the desired record no.

Otherwise how do you account for the wrong client accessing a record no with the right lockCookie? In other words:

clientA has lockCookie1 for record7
clientB has lockCookie2 for record7

Both clients want access to record7 simultaneously.

clientA obtains the lock for record7 first and then is passed back lockCookie1. Now, what is to prevent clientB from using lockCookie1 to work with or unlock record7? Technically, clientB should not get access to lockCookie1, but having a second Hashtable to keep track of the client-lockCookie relationship adds another level of security to the whole locking mechanism. This ensures that only clientA has access to lockCookie1, and that clientB cannot access record7 (until it is unlocked) because clientB does not have lockCookie1.

BTW, locking the entire database is not necessary. You should only account for locking the record in question.

[Alan W Morgan]

Now, what is to prevent clientB from using lockCookie1 to work with or unlock record7?

Ok but how does clientB get lockCookie1 ?

BTW, locking the entire database is not necessary. You should only account for locking the record in question.

What about the case Zee mentioned where I have to reuse deleted records when creating new ones if possible.
clientA does a read() and figures that record 2 is deleted.
Meanwhile clientB goes and deletes record 1
ClientA creates a new record over record 2.
This is not a big deal really but to be correct they should have used record 1.

By locking the whole DB I can avoid this.

What am I missing here ?


Thanks,
Alan.

[ June 21, 2005: Message edited by: Alan W Morgan ]
[ June 21, 2005: Message edited by: Alan W Morgan ]

[David Sham]

Ok but how does clientB get lockCookie1 ?

It does not matter how clientB gets lockCookie1. The fact that you are implementing a solution which would prevent this from happening adds another layer of security. Realistically, clientB should never get lockCookie1. But from a maintainability argument, you could say that by implementing this additional level of security (via a second Hashtable), clientB is prevented from executing a locked-record operation with the wrong lockCookie.

What about the case Zee mentioned where I have to reuse deleted records when creating new ones if possible.
clientA does a read() and figures that record 2 is deleted.
Meanwhile clientB goes and deletes record 1
ClientA creates a new record over record 2.
This is not a big deal really but to be correct they should have used record 1.

By locking the whole DB I can avoid this.

What am I missing here ?

I think you are missing the KISS principle! (Keep It Simple Stupid) Try to keep it as simple as possible. As long as you check to see if a record is deleted (to reuse it) when doing a create new record operation you should be fine.

One suggestion would be to synchronize the entire create new record operation and call a searchForDeletedRecord() method within that operation. If the create new record operation (or method) is synchronized then the executing thread cannot slice out mid-operation. Since it is synchronized, only 1 thread at a time can execute that operation. Therefore, the scenario you described should not occur. This is how I designed my solution.

However, this will not work if you are using multiple Data class instances with each instance using its own RandomAccessFile to read from and write to the database file. I had only 1 Data class and RAF in my solution (which resided in my RMI Remote Implementation object) - because of the requirement to use lockCookies.

[Andrew Monkhouse]

Hi Mihai

What you mean by "implemented the interrupt" ?

I meant that the programmer had to develop code that interrupted the thread doing the locking - it could be argued that the programmer did this in a bad way.

In my locking manager I throw an IntException if a thread which is waiting is interrupted.
So in the other words I have a normal/default behavior and for extreme cases (interrupted cases) i throw exceptions.
Am I right ?

This sounds reasonable.

Regards, Andrew

[Andrew Monkhouse]

Hi Alan,

When I said:

1. I have my wait() in a try catch(InterruptedException) so after interrupt the condition is checked again.

I wasn't 100% clear. What I meant is that my wait is in a try catch and the try catch is inside the while as so:

while (lockMap.containsKey(new Integer(recNo)))
{
try
{
lockMap.wait();
}
catch (InterruptedException inte){}
}

Will I still suffer from the problem you outlined ?

What you are showing here is a case where you are "swallowing" the exception - that is the exception disappears and is never seen again.

While you wont have the same problem that Zee had, you potentially risk loosing marks for doing absolutely nothing with the caught exception - it is considered very bad practice.

Consider the hypothetical scenario that I used to demonstrate the flaw in Zee's code. In the same scenario, the junior programmer doesn't have access to the Data class' code, so they write the same code as I suggested above, try it, and absolutely nothing happens. So they turn on logging and try again - still nothing happens, and nothing appears in the log. So they spend the next 2 days trying to verify to themselves that their concept would have worked (most junior programmers seem to be afraid to go back to the boss and say that their concept is not working and ask for a hint ). So by the time that they finally do go back to the boss 2 days have been wasted, and the boss gets mad at you for having swallowed the exception.

So, as a bare minimum, I personally would like to see a log message stating that the InterruptException was caught.

Regards, Andrew

[Zee Ho]

clientA has lockCookie1 for record7
clientB has lockCookie2 for record7

David, I think we give the locking different definition, yours might be :
To keep write(insert/update/delete) actions in order.
Mine is:
One record can serve only one client.

yours is more effective while my implementation is more simple

If the candidate use the simple solution, he only need to maintain one hashtable(hashmap)
[ June 21, 2005: Message edited by: Zee Ho ]

[Alan Morgan]

What you are showing here is a case where you are "swallowing" the exception - that is the exception disappears and is never seen again.

Andrew,

My code is indeed swallowing the exception but only in this code snippet.
My actual code logs the message and may do more in the future I haven't decided yet. I just left that bit out cause I was focussing more on the try catch being inside the while than the correctness of the whole block.
Guess I should have made that clear.



David,

As regards the two hashmaps...does the KISS principle not come into play here too ?
I mean I can see your solution is more secure but I would have considered it a bit overdoing it...however given your locking score Sun obviously do not.


As for the lock DB. My create() method is already fully synchronized and I have only 1 Data instance.
But does this stop thread from clientA switching out half way thru create() and clientB running delete() to effectively change the ground under clientA's feet as it were ?

[David Sham]

As for the lock DB. My create() method is already fully synchronized and I have only 1 Data instance.
But does this stop thread from clientA switching out half way thru create() and clientB running delete() to effectively change the ground under clientA's feet as it were ?

If your create method is synchronized (in the method signature, public void synchronized create(){}), then yes it should prevent clientA from slicing out during the execution of the create method. This is due to the fact that a synchronized method only allows 1 thread at a time to execute it. Therefore only clientA's thread must complete execution of the create method before clientB's thread can delete a record.

Andrew, please advise if I am wrong here, but that is my understanding of how synchronized code works.

[David Sham]

Wait, wait, wait!! Let me clarify something here. Just by synchronizing the create() method, this will not prevent a different thread from executing the delete() method.

If you have only 1 Data class and RandomAccessFile, then what needs to be synchronized is the read from and write to operations with the RAF. This is because if there is only 1 RAF and its operations are synchronized, then the executing method (create() or delete(), etc.) will have total control over the RAF for the during of the synchronized code enclosing the RAF. If the create() method is synchronized and it handles the RAF operations, then that should prevent a delete from occuring while the create method is executing.

But if the create() method does not handle the RAF operations to the database file, then simply synchronizing it will not prevent a delete() method operation from occuring on the file.

[Alan Morgan]

Originally posted by David Sham:
Wait, wait, wait!! Let me clarify something here. Just by synchronizing the create() method, this will not prevent a different thread from executing the delete() method.

If you have only 1 Data class and RandomAccessFile, then what needs to be synchronized is the read from and write to operations with the RAF. This is because if there is only 1 RAF and its operations are synchronized, then the executing method (create() or delete(), etc.) will have total control over the RAF for the during of the synchronized code enclosing the RAF. If the create() method is synchronized and it handles the RAF operations, then that should prevent a delete from occuring while the create method is executing.

But if the create() method does not handle the RAF operations to the database file, then simply synchronizing it will not prevent a delete() method operation from occuring on the file.

But is this not effectively the same as locking the DB ?
I mean when 1 client is creating in your app can another client be reading ?

[David Sham]

But is this not effectively the same as locking the DB ?
I mean when 1 client is creating in your app can another client be reading ?

If you have only 1 RAF and its operations are synchronized, then only 1 read-from or write-to operation can occur at a time. If the RAF operations are synchronized, this ensures that the thread executing the RAF operation will not slice out mid-operation. So in a sense it is like locking the entire DB, but only for the duration of the execution of the synchronized code. This is why it is not necessary to lock the entire database file if you have only 1 RAF.

If you have multiple RAFs, then you may need to lock the entire database file somehow.