Scjd B&S- Failed due to locking.

[John Matman]

Hello All,
I need your generous help in fixing my locking code. Here is what the evaluator says. I have debugged my code and everything works fine including the locking.

Major point loss for record-locking mechanism, which is not according to spec. Your locking code does not block when trying to lock a locked record.

I have implemented the client side locking. Here is the client side code.

 
public void bookContractor(int recNo, 
                               String[] updateData) throws RecordNotFoundException {
                               
        try {
            if (isLocal) {
                         dataAdapter.bookContractor(recNo, updateData);
             } else {
                         while (rmiData.isLocked(recNo)) {
                         try{
                            Thread.currentThread().sleep(100);
                       
                         } catch (InterruptedException ie) {
                                log("InterruptedException occured" +
                                                          " in read: ");
                         }
 
             }
             rmiData.lock(recNo);
                rmiData.bookContractor(recNo, updateData);
             }
 
        } catch (Exception e) {
            throw new RecordNotFoundException(e.getMessage());
        }finally{
            try{
                if ( !isLocal ) {
                   rmiData.unlock(recNo);
                }
            }catch(Exception e){
                throw new RecordNotFoundException(e.getMessage());
            }
            
        }
     }

[Edit: remainder of post removed. See my post below for reasons why. I will try and clean it up later today and return it. Andrew]
[ July 14, 2008: Message edited by: Andrew Monkhouse ]

[Richard Roehl]

On your client side:

while (rmiData.isLocked(recNo)) {
  try {
    Thread.currentThread().sleep(100);
  } catch (InterruptedException ie) {
  log("InterruptedException occured" +
    " in read: ");
  }
}

Sleeping for 100 ms and then checking to see if the record is still locked violates the spec.

On the server side you need to tell us what the "lock" object is. It appears to be some kind of Collection object but what it really needs to be is a ReaderWriter lock.

[John Matman]

On the server side you need to tell us what the "lock" object is. It appears to be some kind of Collection object

It is a Vector object. If sleeping is off the spec, then the evaluator's comment is confusing. But the spec does not say anything about using the threads sleep method and it is not deprecated.

The evaluators comment is follows

Your locking code does not block when trying to lock a locked record.

[ July 14, 2008: Message edited by: John Matman ]

[Roberto Perillo]

Hey, partner.

Man, I am really sorry for that. Hopefully, you will pass on the next submission.

Did you run some testing on your locking mechanism? Apparently, your code will cause deadlock. For instance, when you unlock a record, you are not calling notify() or notifyAll() to notify waiting Threads that the record locking has been released. Also, the control of your locking mechanism should run on the server side, not in the client side. There, any Thread that wants to lock a record for a client should be put in waiting state (by calling dataObject.wait(); in a while loop), and the unlock() method of your server should call dataObject.notifyAll() after unlocking a record.

Please run some testing with more than 1 Thread on your locking mechanism; start the database server and try to make the different Threads book the same record concurrently to see if you get a deadlock.

[Richard Roehl]

What you have implemented falls more into the category of "spin-lock". What they want you to use is the Object.wait(), notify() and notifyAll() methods. These methods put your method call to sleep for you and don't use any CPU whatsoever as they are held in a queue that requires a notify() interupt to release them. Your code uses CPU every 100ms. This concept is covered in undergrad computer science programs so if you are not a C.S. student you probably won't run into this unless you've specifically read some advanced books on multi-threaded programming. I haven't read the Monkhouse book but I imagine he probably has layman's explanation of the concept. Someone correct me if I'm wrong.

[John Matman]

Hello Robert,
I am not calling threads wait() method. I thinK you call notify or notify all only on the waiting threads. I am using the sleep method which wakes up every 100 milli seconds to check if the record is still locked.


I am guessing i am violating the spec by waking the thread up after every 100 milli seconds whether or not the record is unlocked by the locked thread.

I mean the sleeping thread is consuming the cpu cycles every 100 Ms.

[John Matman]

Richard,
You are right. I am not a CS student. I am a Civil Engg UG.

I got your point and move forward in that direction.

Roberto,
Thanks for your advise too.
J

[John Matman]

Richard,Robert,
I have replace my sleep with wait. please tell if this brings my code to spec?

Client code
--------------

 public void bookContractor(int recNo, 
                               String[] updateData)
                               throws RecordNotFoundException {
                               
        try {
            if (isLocal) {
                dataAdapter.bookContractor(recNo, updateData);
             } else {
                while (rmiData.isLocked(recNo)) {
                   try{
                            Thread.currentThread().wait();
                   } catch (IllegalMonitorStateException ie) {
                                System.out.println("InterruptedException" +
                                        " occured in read: ");
                   }
 
                }
                rmiData.lock(recNo);
                rmiData.bookContractor(recNo, updateData);
             }
 
        } catch (Exception e) {
            throw new RecordNotFoundException(e.getMessage());
        }finally{
            try{
                if ( !isLocal ) {
                   rmiData.unlock(recNo);
                }
            }catch(Exception e){
                throw new RecordNotFoundException(e.getMessage());
            }
            
        }
     }

Here is my unlock method from data class.

Database code
----------------

 
 public synchronized void unlock(int recNo) throws RecordNotFoundException {
        lock.remove(new Integer(recNo));
        notifyAll();
    }

[ July 14, 2008: Message edited by: John Matman ]

[Edit: Put code inside UBB code tags - Andrew]
[ July 14, 2008: Message edited by: Andrew Monkhouse ]

[Richard Roehl]

Sorry, no that change won't work for two reasons: 1) the wait has to be on the locked object and not the current thread. 2) The wait has to be implemented on the server side code and not the client. Ask Andrew Monkhouse (the moderator) if his book on SCJD will be enough for you to get a clear idea of what Sun is expecting.

Richard

[John Matman]

Guys,
Thanks much for your answers. I did the following changes. Does this sound oK?

client code
----------------

 
        try {
            if (isLocal) {
                dataFacade.bookContractor(recNo, updateData);
             } else {
                
                rmiData.lock(recNo);
                rmiData.bookContractor(recNo, updateData);
             }
 
        } catch (Exception e) {
            throw new RecordNotFoundException(e.getMessage());
        }finally{
            try{
                if ( !isLocal ) {
                   rmiData.unlock(recNo);
                }
            }catch(Exception e){
                throw new RecordNotFoundException(e.getMessage());
            }
 
        }

And my database code for lock , islocked, unlock is

    public void lock(int recNo) throws RecordNotFoundException {
        
        synchronized(lock){
            while(isLocked(recNo)){
                try{
                    
                    lock.wait();
                }catch(InterruptedException e){
                    
                    System.out.println("Error occured: "+e.getMessage());
                }catch(IllegalMonitorStateException ime){
                    
                    System.out.println("Error occured: "+ime.getMessage());
                }catch(IllegalArgumentException iae){
                    
                    System.out.println("Error occured: "+iae.getMessage());
                }
                        
                        
            }
            
            lock.add(recNo);
            
        }
        //lock.add(recNo);
    }

I did not synchronize my isLocked method.


[Edit: removed code that was beyond what our policy allows. Placed code inside UBB code tags. Andrew]
[ July 14, 2008: Message edited by: Andrew Monkhouse ]

[Andrew Monkhouse]

I have removed most of the postings in this topic pending cleanup. Posting your entire locking methodology and your client side code is well outside what we consider reasonable. Please read What is the policy on posting questions I saw on the exam / details of how to do the assignment? in the JavaRanch SCJD FAQ for more information.

Andrew

[Andrew Monkhouse]

Reopened now.

In your lock method, what throws IllegalArgumentException?

Can IllegalMonitorStateException ever be thrown? And if not, is there value in having that code there?

If you are going to use a Vector and try to catch every possible exception, then you should probably be catching ConcurrentModificationException (and no - I am not advocating catching all exceptions, and I don't think a Vector is a good choice (see "difference between Vector & ArrayList" for some commentary. Or Phillipe Maquet's commentary on Vectors in "Blocks synchronized, but without wait() or notifyAll() methods."))

To make it more explicit, how are you ensuring that only the client who locks a record can unlock it? (Check your instructions - it may have a requirement about this - possibly in the documentation provided :roll: with the supplied interface. With what you have now, I believe I could write unscrupulous client code that does not honor the locking:

Client A locks record 5

Client B starts by unlocking record 5 (don't care if it is locked or not - exception will be swallowed)

Client B locks record 5

To be honest I don't see this as being a big issue - if we are trying to guard against unscrupulous client software then we will need to write the business logic on the server (which I also disagree with ). But if your instructions state that only the client who locks a record is allowed to unlock it then you probably need something other than a Vector.

Regards, Andrew

[John Matman]

Andrew,
First of all sorry for posting my code. Thought my failed code would not appeal anybody.

Can IllegalMonitorStateException ever be thrown?

True. I will remove the not needed exceptions.

I don't think a Vector is a good choice

I used Vector because it is synchronized. I would consider changing it to ArrayList.

how are you ensuring that only the client who locks a record can unlock it?

All client calls goes through the interface.

The client does the lock,update,unlock as an atomic operation .

Example

Client A locks the record 5 , as well as the whole collection so no other thread is allowed to lock any record.My lock method synchronizes the whole collection object.

Client A performs the update/delete..etc and calls the unlock which removes the record no from collection and notifies other threads that the collection is unlocked for other threads use.

Client A locks record 5
* Client B starts by unlocking record 5 (don't care if it is locked or not - exception will be swallowed)
* Client B locks record 5

Andrew,
I can sense what you are saying. If i had move my locking completely
to server, then it is going to be a rewrite. I have documented that all clients should get the lock first before doing update,delete etc.
[ July 15, 2008: Message edited by: John Matman ]

[Graham Williamson]

Hi John

Can I make a suggestion, and please don't take this the wrong way, but if you haven't already, I'd really suggest reading Andrew's book.

It'll fill in all the knowledge gaps you might have on threading, synchronization and locking mechanisms/methodologies. Then you'll have all the knowledge at your fingertips. It's probably one of the best computing books I've ever invested in, as it assumes a pretty high level of Java knowledge already, focusing instead on the areas you'll need to know for the assignment.

I literally finished reading it on the train this morning, and it won't take you long to race through it.

Now I just need to develop my solution, starting tonight!

Best regards
Graham