Appealing grading results?

[Jaro Simak]

Hi,
Is it possible to appeal the grading result?

:-) yes, I failed. Zero points in locking. But, I don't think the
tester is right. The explanation - "Your locking method does not block when trying to lock a locked record." - is not true.
Pretty arrogant from me, hmm? :-)
I believe what I see in the code and it is a classic wait-pattern.

What should I do :-)
Greetings
Jaro

[Mark Smyth]

Originally posted by Jaro Prospal:
Hi,
Is it possible to appeal the grading result?

:-) yes, I failed. Zero points in locking. But, I don't think the
tester is right. The explanation - "Your locking method does not block when trying to lock a locked record." - is not true.
Pretty arrogant from me, hmm? :-)
I believe what I see in the code and it is a classic wait-pattern.

What should I do :-)
Greetings
Jaro

Can you put up your locking methods?

[Chulwoo Choi]

The explanation - "Your locking method does not block when trying to lock a locked record." - is not true.
Pretty arrogant from me, hmm? :-)
I believe what I see in the code and it is a classic wait-pattern.

Did you test your locking?
You can write a test code in which:

one thread calls the lock a record.

then, another thread tries lock the same record (when the first thread hasn't called unlock). And you must see the status of this thread (using getState() method) to be WAITING.

If it still works fine, then, you can contact Sun.

Good luck.
Chulwoo

[Jaro Simak]

Thanks for your interest.
Well, maybe I am going to blame myself, but after all I did it already :-)


Here is the piece from Data class.
The synchronizes on the member lockedRecords. If it gets the monitor, it checks if the record
is locked. The method waitUntilRecordIsNotLocked returns true if the record is unlocked and
false is already locked by the same thread (for instance by calling the lock method twice).
I have to say, the implementation of the sequence lock, update, unlock reside on the server
("thin" client), so I identify the clients by the thread reference.
Data class is singleton.

    /**
     * the currently locked records are stored here.
     */
    private LockMap lockMap = new LockMap();
 
 
    public long lockRecord(final long recNo) throws RecordNotFoundException {
        Long rn = new Long(recNo);
        synchronized (lockMap) {
            boolean isUnlocked = lockMap.waitUntilRecordIsNotLocked(rn);
            // if this line is reached the record is unlocked (isUnlocked==true)
            // or already locked by the same thread (isUnlocked==false)
            Long cookie;
            if (isUnlocked) {
                log.finer("The record " + recNo + " is not locked");
                cache.readRecord(rn);
                cookie = lockMap.put(rn);
                log.fine("Record " + recNo + " is locked now. Cookie = "
                        + cookie);
            } else {
                cookie = lockMap.get(rn);
                log.fine("Record " + recNo + " is already locked by the "
                        + " current thread thread. Cookie = " + cookie);
            }
            return cookie.longValue();
        }
    }
 
 
 
The piece of code from the class LockMap.
(The synchronized modifier of waitUntilRecordIsNotLocked has no sense because the threads 
synchronizes on lockedRecord, but it is not wrong.)
The class LockValue is just a pair [cookie, thread reference=owner]
 
 
    private Map lockedRecords = new HashMap();
 
    /**
     * The method checks whether the specified record is locked by a different
     * thread. If it is the case the calling thread goes to the waiting state.
     * After a notification the lock condition is checked anew until the record
     * is found to be unlocked. The method returns <code>true</code> than.
     * If the thread is locked but the thread that caused the lock is
     * the same as the current thread. The method returns false in this
     * particular case.
     * This method assures the reenterablity of locks.
     * @param recNo record number
     * @return <code>true</code> when the specified record is unlocked;
     * <code>false</code> when the specified record is already locked by the
     * same thread.
     */
    synchronized boolean waitUntilRecordIsNotLocked(final Long recNo) {
        LockValue lock = null;
        while ((lock = (LockValue) lockedRecords.get(recNo)) != null) {
            if (lock.getLockOwner() == Thread.currentThread()) {
                log.finer("The record " + recNo + " is already locked by this "
                        + "thread. No need to wait.");
                return false;
            }
            try {
                log.finer("The record " + recNo + " is locked ... waiting");
                wait();
            } catch (InterruptedException e) {
                // don't care, let the thread check the lockedRecord again
                log.warning("The waiting thread " + Thread.currentThread()
                        + " has been interrupted.");
            }
        }
        return true;
    }
 

I really don't know what is there wrong.

[Andrew: put code between [code] and [/code UBB tags]
[ November 09, 2006: Message edited by: Andrew Monkhouse ]

[Mark Smyth]

Originally posted by Jaro Prospal:
Thanks for your interest.
Well, maybe I am going to blame myself, but after all I did it already :-)


Here is the piece from Data class.
The synchronizes on the member lockedRecords. If it gets the monitor, it checks if the record
is locked. The method waitUntilRecordIsNotLocked returns true if the record is unlocked and
false is already locked by the same thread (for instance by calling the lock method twice).
I have to say, the implementation of the sequence lock, update, unlock reside on the server
("thin" client), so I identify the clients by the thread reference.
Data class is singleton.

    /**
     * the currently locked records are stored here.
     */
    private LockMap lockMap = new LockMap();
 
 
    public long lockRecord(final long recNo) throws RecordNotFoundException {
        Long rn = new Long(recNo);
        synchronized (lockMap) {
            boolean isUnlocked = lockMap.waitUntilRecordIsNotLocked(rn);
            // if this line is reached the record is unlocked (isUnlocked==true)
            // or already locked by the same thread (isUnlocked==false)
            Long cookie;
            if (isUnlocked) {
                log.finer("The record " + recNo + " is not locked");
                cache.readRecord(rn);
                cookie = lockMap.put(rn);
                log.fine("Record " + recNo + " is locked now. Cookie = "
                        + cookie);
            } else {
                cookie = lockMap.get(rn);
                log.fine("Record " + recNo + " is already locked by the "
                        + " current thread thread. Cookie = " + cookie);
            }
            return cookie.longValue();
        }
    }
 
 
 
The piece of code from the class LockMap.
(The synchronized modifier of waitUntilRecordIsNotLocked has no sense because the threads 
synchronizes on lockedRecord, but it is not wrong.)
The class LockValue is just a pair [cookie, thread reference=owner]
 
 
    private Map lockedRecords = new HashMap();
 
    /**
     * The method checks whether the specified record is locked by a different
     * thread. If it is the case the calling thread goes to the waiting state.
     * After a notification the lock condition is checked anew until the record
     * is found to be unlocked. The method returns <code>true</code> than.
     * If the thread is locked but the thread that caused the lock is
     * the same as the current thread. The method returns false in this
     * particular case.
     * This method assures the reenterablity of locks.
     * @param recNo record number
     * @return <code>true</code> when the specified record is unlocked;
     * <code>false</code> when the specified record is already locked by the
     * same thread.
     */
    synchronized boolean waitUntilRecordIsNotLocked(final Long recNo) {
        LockValue lock = null;
        while ((lock = (LockValue) lockedRecords.get(recNo)) != null) {
            if (lock.getLockOwner() == Thread.currentThread()) {
                log.finer("The record " + recNo + " is already locked by this "
                        + "thread. No need to wait.");
                return false;
            }
            try {
                log.finer("The record " + recNo + " is locked ... waiting");
                wait();
            } catch (InterruptedException e) {
                // don't care, let the thread check the lockedRecord again
                log.warning("The waiting thread " + Thread.currentThread()
                        + " has been interrupted.");
            }
        }
        return true;
    }
 

I really don't know what is there wrong.

[Andrew: put code between [code] and [/code UBB tags]

[ November 09, 2006: Message edited by: Andrew Monkhouse ]

Must say if found your code a slightly more complicated than I think it needed to be, and you should probably also have a notifyAll() at the end of the lock method to inform other threads that you are finished with the monitor.

Having said all that I tried your code (with a few tweaks to get it running cos I dont have all your code ie the unlock methods). I ran 6 threads calling lock(0) without unlocking and got the following output. As you can see there are five threads waiting patiently forever to get a lock that will never be released.

 
In lock[Thread-0,5,main]
locked [Thread-0,5,main]
In lock[Thread-1,5,main]
waiting [Thread-1,5,main]
In lock[Thread-2,5,main]
waiting [Thread-2,5,main]
In [Thread-3,5,main]
waiting [Thread-3,5,main]
In lock[Thread-4,5,main]
waiting [Thread-4,5,main]
In lock[Thread-5,5,main]
waiting[Thread-5,5,main]

I ran this code and added println statements to your methods;

 
public class Test {
    static LockManager locks;
   
    public static void main(String[] args) {
locks= new LockManager();
LockThread[] t=new LockThread[6];

for (int i=0;i<t.length;i++){
    Thread t1=new LockThread();
    t1.start();
}
    }
    
    static class LockThread extends Thread{
public void run(){
    try {
locks.lock(0);
sleep(1000);
    } catch (InterruptedException e) {
e.printStackTrace();
    }
}
    }
 
}
   

Without having seen all the implementation I think that you definately have grounds for an appeal as it definately blocks other threads while the record is locked. 0 / 40 is an unfair score and the reason given for it is false, so go for it.

Best of luck with it,

Mark

[ November 10, 2006: Message edited by: Mark Smyth ]
[ November 10, 2006: Message edited by: Mark Smyth ]

[Jaro Simak]

Thank you very much for spending so much time with the problem. Thanks for the putting the UBB Codes inside the posts as well. Next time I will take care of it.

It is good to hear that I am right.
It took 10 week to grade the assignment, anyway. And for this result

[Andrew Monkhouse]

Hi Jaro,

Sorry - ran out of time yesterday, so didn't respond (and now I am running late for work, so this will have to be short as well).

I think you do have reason to appeal the decision, as you are blocking where you need to.

Technically the assignment does not suggest that you should allow the same record to be locked twice by the same client. So I guess if someone wanted to be pedantic about it, they could claim that you are not blocking in that case. But if you did, you would create a scenario where a lock could never be released. So I think your way of handling this is a good design decision.

It looks like your thread will get the mutex on the LockMap object twice while performing a lock operation - while this wont cause a problem, it is generally not desirable.

But none of those should cause a total loss of marks.

If you do decide to appeal, be aware that in the past we have seen that it takes quite a while to get the appeal processed, and you may have to send multiple emails to ensure that it continues to be worked on. Good luck with it though.

Originally posted by Mark Smyth:
[...] and you should probably also have a notifyAll() at the end of the lock method to inform other threads that you are finished with the monitor. [...]

There's no need for that - you only need to notify other threads when there is a reason for them to re-check whether their lock is now available - in other words, at the end of your unlock() method.

Regards, Andrew

[Jeroen T Wenting]

Allowing a thread to lock its own locked records again might indeed be the cause of the failure you got.
Technically the assessor is of course correct there, if he wants to be extremely strict.
I think throwing an exception ("attempting to lock a record already locked by this thread" or some such) would have been better.

Personally I'd not have rejected a solution based on that alone, but I don't have the guidelines Sun's assessors have to work to at my disposal (of course).

[Tim LeMaster]

I'm not yet working on my SCJD but I did also get your code working after filling in the gaps (of course I didn't have your other classes). So baring in major differences there, It also worked for me.

As to the thread relocking an already locked record. Unless its in the spec about what to do with that, I like your solution. Whats the harm in trying to relock something already locked? Why bother with an exception?

But from the explanation - "Your locking method does not block when try to lock a locked record" - that is indeed true if its the same thread - but that seems undesirable to me - as how would it ever get unlocked?

I agree with no need for notifyAll at the end of lock. There isn't a reason for the other threads to check locks at this point - nothing is newly unlocked.

Good luck!

[Jaro Simak]

Hi,
thanks a lot for your interest.

My assignment (B&S 2.2.3) defines the lock method as follows:

   
// Locks a record so that it can only be updated or deleted by this client.
// Returned value is a cookie that must be used when the record is unlocked,
// updated, or deleted. 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.
public long lockRecord(long recNo)
throws RecordNotFoundException;
 

The reason that I prohibited to lock a record that is already locked by the same thread was the formulation "different thread". I guess the implementation is correct.

It looks like your thread will get the mutex on the LockMap object twice while performing a lock operation - while this wont cause a problem, it is generally not desirable.

Yes, the synchronized modifier in the method waitUntilRecordIsNotLocked
is an overhead, it is not necessary.

Greetings
Jaro
[ November 11, 2006: Message edited by: Jaro Prospal ]

[Mihai Radulescu]

Hi, People

I just read the post and I hope I not to late with my reply.
I think that the main problem is the "unique client id",
In the code snippet you can see that the Lock Manager uses the pair [cookie, Thread]. This logic may work if every client has only one thread but if we deal with RMI we lose the control over the threads, there is no guarante
e that every client will use a separate thread(this theme was enough discussed on the ranch).
This only if the RMI is used.

So lets say that the RMI daemon uses only one thread(only hypothetic) to solve its remote requests. Will be something like :

client 1 - \
client 2 - - RMI - run request(1, 2, 3)
client 3 - /

Three clients (with three different threads) send request to the RMI . The RMI uses a single thread to solve all this requests.
So if client 1 locks record1, the Lock manager stores [record 1, RMI Thread]
if client 2 locks the record 1, the lock manager think that the same thread (the RMI Thread) and it returns false and this is wrong because the Record 1 is already locked (from Client 1).
If you do a simple multithread test (like Mark suggest) where every client will have the same thread, the test will be successful.

Regards M.
[ November 15, 2006: Message edited by: Mihai Radulescu ]

[Jaro Simak]

Hi Radu,
yes, you right and I was thinking about this case.

But,in my case, all the lock, update, unlock logic resides on the server. Some folks here call it "thin" client.
Thus, a thread processes the three calls for one request and then can go to process other ones.

On the other side, the testers can test the code the way you described - remote calls of lock and unlock from
clients. In that case, it may happen that the code does not work, as you described. But, these tests seems to
me against the claim in the assignment:

You may assume that at any moment, at most one program is accessing the database file;
therefore your locking system only needs to be concerned with multiple concurrent clients of your server.

The tests can be considered as another server and for that server the locking mechanism is not required to work.

Another point is, I have no idea how to transport some client IDs through the API.

Greeting
Jaro

[Mike Ngo]

remote calls of lock and unlock from
clients.

I hope they don't do this. In real application, this is a disaster. I am going to discuss this in choices.txt to make sure they understand that.

[Mark Smyth]

Originally posted by Jaro Prospal:
Hi Radu,
yes, you right and I was thinking about this case.

But,in my case, all the lock, update, unlock logic resides on the server. Some folks here call it "thin" client.
Thus, a thread processes the three calls for one request and then can go to process other ones.

On the other side, the testers can test the code the way you described - remote calls of lock and unlock from
clients. In that case, it may happen that the code does not work, as you described. But, these tests seems to
me against the claim in the assignment:

The tests can be considered as another server and for that server the locking mechanism is not required to work.

Another point is, I have no idea how to transport some client IDs through the API.

Greeting
Jaro

I think that your thin client approach is a valid one (which I am also using) and has been used in the past by others succesfully to pass the SJCD. To be honest I think the reason you failed is because the examiner did not understand (or look at) your wait method in the code or else did not investigate it in enough detail. His comments when failing you only mentioned that the lock method did not block which it clearly does and he made no mention of any other threading issue. I would go ahead with the appeal and I am certain that you will be passed on reconsideration. Good luck and don't take no for an answer.

Mark

[Mihai Radulescu]

Jaro,

You don't need to transport the client unique id.
You can use a remote factory - this theme was to much discussed here, just do a search using remote factory or peek in the Andrew book - he uses the same "trick".

Mark,

You do some serious assumption here,

has been used in the past by others succesfully to pass the SJCD

I don't think that "thread like id" has ever a chance to pass the SJCD.
You can try and risk.

Regards M.