About:My URLyBird1.3.2 Locking

[xi ruo]

I use a HashMap to lock the threads.
My lock/unlock/isLocked methods,code in Data.java which implements DBMain following:

private static int DATABASE_LOCK=-1;
  public void lock(int recNo) throws RecordNotFoundException {
    Integer recordNumber = new Integer(recNo);
    synchronized (mapLocks) {
      if (recNo == DATABASE_LOCK) {
        while (!mapLocks.isEmpty()) {
          try {
            System.out.println(Thread.currentThread().getName()+":"+this.toString() + " Waiting for no locks."+ recNo);
            mapLocks.wait();
          }
          catch (InterruptedException e) {
            throw new RecordNotFoundException(UNEXPECTED+":Interrupt ocurred. Could not complete lock.");
          }
        }
      }
      else {
        while (mapLocks.containsKey(recordNumber) ||
               mapLocks.containsKey(new Integer(DATABASE_LOCK))) {
          try {
            System.out.println(Thread.currentThread().getName()+":"+this.toString() + " Waiting for record unlock."+ recNo);
            mapLocks.wait();
          }
          catch (InterruptedException e) {
            throw new RecordNotFoundException(UNEXPECTED+":Interrupt ocurred. Could not complete lock.");
          }
        }
      }
      System.out.println(Thread.currentThread().getName()+":"+this.toString() + " locking  record." + recNo);
      mapLocks.put(recordNumber, this);
    }
  }

  public void unlock(int recNo) throws RecordNotFoundException {
    Integer recordNumber = new Integer(recNo);
    synchronized (mapLocks) {
      while (mapLocks.get(recordNumber) != null &&
             mapLocks.get(recordNumber).equals(this)) {
        mapLocks.remove(recordNumber);
        System.out.println(Thread.currentThread().getName()+":"+this.toString() + " Unlocked the record." + recNo);
        mapLocks.notifyAll();
      }
    }
  }

  public boolean isLocked(int recNo) throws RecordNotFoundException {
    Integer recordNumber = new Integer(recNo);
    synchronized(mapLocks) {
      return mapLocks.get(recordNumber) != null&&mapLocks.get(recordNumber).equals(this);
    }

AnotherDBMain implemented by suncertify.db.DataAdapter client end,not by suncertify.remote.DatabaseImpl server end,the segment code that lock/unlock a record in data file following:

Data db=new Data("suncertify_db/db-1x2.db");
  public synchronized String[] read(int recNo) throws RemoteException,RecordNotFoundException {
    try{
      String[] s;
      db.lock(recNo);
      s=db.read(recNo);
      db.unlock(recNo);
      return s;
    }catch(RecordNotFoundException e){
      throw new RecordNotFoundException(e.getMessage());
    }
  }
  public synchronized void update(int recNo,String[] data) throws RemoteException,RecordNotFoundException {
    try{
      db.lock(recNo);
      db.update(recNo,data);
      db.unlock(recNo);
    }catch(RecordNotFoundException e){
      throw new RecordNotFoundException(e.getMessage());
    }
  }
  public synchronized void delete(int recNo) throws RemoteException,RecordNotFoundException {
    try{
      db.lock(recNo);
      db.delete(recNo);
      db.unlock(recNo);
    }catch(RecordNotFoundException e){
      throw new RecordNotFoundException(e.getMessage());
    }
  }
  public synchronized int[] find(String [] criteria) throws RemoteException,RecordNotFoundException {//can I do like this???
    try{
      int[] recNoArray;
      db.lock(-1);//DATABASE_LOCK
      recNoArray=db.find(criteria);
      db.unlock(-1);//DATABASE_UNLOCK
      return recNoArray;
    }catch(RecordNotFoundException e){
      throw new RecordNotFoundException(e.getMessage());
    }
  }
  public synchronized int create(String [] data) throws RemoteException,DuplicateKeyException {
    try{
      int recNo;
      db.lock(-1);
      recNo=db.create(data);
      db.unlock(-1);
      return recNo;
    }catch(RecordNotFoundException e){
      throw new DuplicateKeyException(e.getMessage());
    }
  }

[Vlad Rabkin]

Hi Xi,
To my PERSONAL opinion it is not Ok, since user lock(lock/unlock/isLocked) is something different from aynchronization locks.
If a user has locked a record, but has not started updating it, another user should be able to read the record, which is not possible in your implementation.
The Interface says:
// Locks a record so that it can only be updated or deleted by this client.
It says nothing about read, find, create methods.
I know that one person, which had something simular to your design had got 100% for locking, but:
1. I am not sure that really had this design
2. I am not sure that Sun tester has carefully looked at his code.
I discussed the design with Andrew. My personal opinion it is a design which will can lead to get max scores, but which can lead you to fail.
I don't know
Could send comment in interface for you lock methods?
Max, please take a part in the topic!!!
Best,
Vlad

I am not really sure
[ September 11, 2003: Message edited by: Vlad Rabkin ]
[ September 11, 2003: Message edited by: Vlad Rabkin ]

[xi ruo]

Hi Vlad:
thanks for your comment.

My personal opinion it is a design which will can lead to get max scores, but which can lead you to fail.

I am afraid to fail out of life.

If a user has locked a record, but has not started updating it, another user should be able to read the record, which is not possible in your implementation.

It's correct.

The Interface says:
// Locks a record so that it can only be updated or deleted by this client.
It says nothing about read, find, create methods.

Is it said "I can only implement the lock/unlock methods for the update and delete method,another methods ignore that."?
To your opinion,How should I rework my lock/unlock/isLocked methods to let them accord with the explanation for these methods.
otherwise,How should I using the isLocked method?
thanks for all response.
[ September 11, 2003: Message edited by: xi ruo ]

[John Smith]

private static int DATABASE_LOCK=-1;
Should be declared as "private static final int".

public void lock(int recNo) throws RecordNotFoundException {
Integer recordNumber = new Integer(recNo);
synchronized (mapLocks) {

Synchronize the entire method, instead of the block, -- it will be much more readable and 99.99% of the time spent in your method is spent in the synchronized block anyway.

catch (InterruptedException e) {
throw new RecordNotFoundException(UNEXPECTED+":Interrupt ocurred. Could not complete lock.");
}

This is bad. You are mapping the InterruptedException into a RecordNotFoundException, which is misleading to the caller of the method.
mapLocks.put(recordNumber, this);
Which object is pointed to by "this"? You said that your methods are part of Data, so "this" must be an instance of Data?
while (mapLocks.get(recordNumber) != null && mapLocks.get((recordNumber).equals(this)
There is no reason to call mapLocks.get(recordNumber) twice. Also the while loop is misleading here, -- you are not iterating and you are not waiting on a condition. A simple "if" will do it.
[ September 11, 2003: Message edited by: Eugene Kononov ]

[Vlad Rabkin]

Hi Xi,

Is it said "I can only implement the lock/unlock methods for the update and delete method,another methods ignore that."?

That is how I understand URLyBird 1.1.1 assignement.
Could you please send descption of lock/unlock/isLocked method in your assignmennt? It seems our projects are a bit different: I don't have isLocked() and I don't have database lock for lock/unlock.
Best,
Vlad

[xi ruo]

Hi Eugene,Thank you.

private static int DATABASE_LOCK=-1;
Should be declared as "private static final int".

Sorry,it's my carelessness.In my code,I declare as this.

public void lock(int recNo) throws RecordNotFoundException {
Integer recordNumber = new Integer(recNo);
synchronized (mapLocks) {

Synchronize the entire method, instead of the block, -- it will be much more readable and 99.99% of the time spent in your method is spent in the synchronized block anyway.

Is synchronize the entire method the same as synchronize the static instance of HashMap which is called "mapLocks"?

catch (InterruptedException e) {
throw new RecordNotFoundException(UNEXPECTED+":Interrupt ocurred. Could not complete lock.");
}

This is bad. You are mapping the InterruptedException into a RecordNotFoundException, which is misleading to the caller of the method.

I rework this following:

catch (InterruptedException e) {
System.out.println("Interrupt ocurred. Could not complete lock.");
}

mapLocks.put(recordNumber, this);
Which object is pointed to by "this"? You said that your methods are part of Data, so "this" must be an instance of Data?

It's correct."this" is an instance of Data.

while (mapLocks.get(recordNumber) != null && mapLocks.get((recordNumber).equals(this)
There is no reason to call mapLocks.get(recordNumber) twice. Also the while loop is misleading here, -- you are not iterating and you are not waiting on a condition. A simple "if" will do it.

Rework following:

while(this.equals(mapLocks(recordNumber)))

About "if" statement or "while" statement,there is a segment saying by Andrew

[xi ruo]

Hi Vlad,descption of lock/unlock/isLocked method in my assignmennt:
Required Interface

Your data access class must be called "Data.java", must be in a package called "suncertify.db", and must implement the following interface:
package suncertify.db;
public interface DBMain {
// Reads a record from the file. Returns an array where each
// element is a record value.
public String [] read(int recNo) throws RecordNotFoundException;
// Modifies the fields of a record. The new value for field n
// appears in data[n].
public void update(int recNo, String [] data)
throws RecordNotFoundException;
// Deletes a record, making the record number and associated disk
// storage available for reuse.
public void delete(int recNo) throws RecordNotFoundException;
// Returns an array of record numbers that match the specified
// criteria. Field n in the database file is described by
// criteria[n]. A null value in criteria[n] matches any field
// value. A non-null value in criteria[n] matches any field
// value that begins with criteria[n]. (For example, "Fred"
// matches "Fred" or "Freddy".)
public int [] find(String [] criteria)
throws RecordNotFoundException;
// Creates a new record in the database (possibly reusing a
// deleted entry). Inserts the given data, and returns the record
// number of the new record.
public int create(String [] data) throws DuplicateKeyException;
// 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;
// Releases the lock on a record.
public void unlock(int recNo) throws RecordNotFoundException;
// Determines if a record is currenly locked. Returns true if the
// record is locked, false otherwise.
public boolean isLocked(int recNo)
throws RecordNotFoundException;
}
Any unimplemented exceptions in this interface must all be created as member classes of the suncertify.db package. Each must have a zero argument constructor and a second constructor that takes a String that serves as the exception's description.
Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.

Locking

Your server must be capable of handling multiple concurrent requests, and as part of this capability, must provide locking functionality as specified in the interface provided above. 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. Any attempt to lock a resource that is already locked should cause the current thread to give up the CPU, consuming no CPU cycles until the desired resource becomes available.

[Vlad Rabkin]

Hi Xi and Eugene,
Eugene:

This is bad. You are mapping the InterruptedException into a RecordNotFoundException, which is misleading to the caller of the method.

Eugene, Unfortunatelly the method in interface (definded by Sun) of URLyBird has no other exceptions to be thrown.
1) I throw my RuntimeException in case of InterruptedException.
2) Some people throw RecordNotFoundException or its subclassed checked exception rejecting to consider Interrupted exception as an RuntimeException.
3) Some people ignore it at all and just log the problem on the server.
I don't like 2 (as you) and 3, but it seems that at least solution 2 is also OK, since there have people who got perfect scrores by doing it.
So, I would not so critisize solution 2. I find it at least better than 3, since I beleive the client must be somehow notified about the problem.

Best,
Vlad

[Vlad Rabkin]

Hi Xi,
Well, we have different interfaces: you don't have cookies and you do have isLocked method.
I don't really understand why isLocked method, since it is usefull only for internal use in the class which implements your this interface.
Still, there is following comment for lock method:

// 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.

Does it says that "Locks a record so that it can only be READ by this client." ? No.
Xi, I am not able to provide with the clear answer. I personally wouldn't use such chain: lock-read-unlock. I beleive lock/unlock is usefull for such things like booking a room : lock-read-update-unlock (you read there to make sure the record is still available after you acquired a lock).
It is my personal opinion and there is no way I want to prevent you from doing what you suggested.
I still hope that somebody else can provide you with more definite response.
Just be very carefull there!
Best,
Vlad

[John Smith]

while(this.equals(mapLocks(recordNumber)))
About "if" statement or "while" statement,there is a segment saying by Andrew
I am not sure what segment you refer to. The "while" loop implies iteration which you don't have, and therefore the "if" would be much more expressive here.

[xi ruo]

Hi Vlad,
My booking room method is in the suncertify.gui.GUIController class,implement following:
DataAdapter is the class which provide read/update/find/create/delete methods

DataAdapter connection=new DataAdapter();
  public void bookSelectedRecord(JTable dbTable) throws GUIControllerException {
    try {
      JComponent component=(JComponent)dbTable.getRootPane();
      int index = dbTable.getSelectedRow();
      if ( (index >= 0) && (index < dbTable.getRowCount())) {
        int[] selectedRecord = connection.find(new String[] {
                                               (String) dbTable.getValueAt(index, 0),
                                               (String) dbTable.getValueAt(index, 1)
        });
        if (selectedRecord.length != 1) {
          throw new RuntimeException("Databse file format error.");
        }
        String inputNumber = JOptionPane.showInputDialog(component,"Please Enter A 8 Digit Number", "Input",JOptionPane.QUESTION_MESSAGE);
        if (inputNumber == null)
          return;
        if (inputNumber.length() != 8) {
          JOptionPane.showMessageDialog(component,"The number entered is not a 8 digit number.", "Error message.",JOptionPane.ERROR_MESSAGE);
          return;
        }
        try {
          Integer.parseInt(inputNumber);
        }
        catch (Exception e) {
          JOptionPane.showMessageDialog(component,"The number entered is not a 8 digit number.", "Error message.",JOptionPane.ERROR_MESSAGE);
          return;
        }
        String[] values = connection.read(selectedRecord[0]);
        values[values.length - 1] = inputNumber;
        connection.update(selectedRecord[0], values);
        dtm.setValueAt(inputNumber, index, values.length - 1);
      } else {
        JOptionPane.showMessageDialog(component, "Select operation failed.","Error message.", JOptionPane.ERROR_MESSAGE);
      }
    }catch(Exception e){
      throw new GUIControllerException(e);
    }
  }

Booking a room:
method-chain like this: find-->read-->update
then,lock-chain like this: lock(-1)-->find-->unlock(-1)-->lock(recordNumber)-->read-->unlock(recordNumber)-->lock(recordNumber)-->update-->unlock(recordNumber)

[xi ruo]

About "if" statement or "while" statement,there is a segment saying by Andrew:

For the unlock, you need to know that you own the lock, and if so, you need to unlock it. Even if this thread was sliced out after the if statement was evaluated as true and before the resulting actions take place, nothing will have changed - no-one else could have removed your lock. Maybe coding a while statement will be better for future enhancements (I cant see how in context), but my personal feeling is that the if statement is slightly more readable in this case.

[xi ruo]

My booking method is so bad that cause that my sense babelism.
I sincerely consult a good way for the booking method.

[Vlad Rabkin]

Hi Xi,
Let's try to do everything step by step. Forget about GUIClient.
I would first finish with the server side.

method-chain like this: find-->read-->update

It looks like you have decided to call lock/unlock not on the client , but on the server (in your DataAdapter class).
Sequence on client find-->read-->update causes following on the server (in you implementation):
lock(-1)-find-unlock(-1)-lock(X)- read(X) - unlock(X)-lock(X) -update(X)-unlock(X). Right?
If so, you forgot to check in your update method on the server if the room still available, because betweem you read and update somebody could have booked the record!
That is actually why you need lock method:
1) lock record (everybody should be able to read a record, but no other client may change it)
2) read the record to check if the romm is still available to be booked
3) update the record
4) unlock the record
If any other client attempts to change the record at the same time he will wait (his lock call will block till the first client unlocks the record)
You have to decide where you check if the room is still available. Since you hided lock/unlock from the client your update method in adapter must do it before update record, otherwise following can happen:
Room #3 is available
1. Client 1 reads the record and knows now that the record is available
2. Client 2 reads the record and decided to book it. It is not available anymore
3. Client 1 decides also to book the room and do succesfully. It shouldn't happen, because the Client 2 has booked already the record!
Did you get the idea?
Best,
Vlad

[Andrew Monkhouse]

Hi Xi,
When referring to something somebody else was talking about, it is nice to also post a link to the thread where the discussion was being held. That way people can read the entire text in context if they want to.
In the section you quoted from me, from memory (I cannot find the post anymore ) I was discussing with Max whether we should use an "if" or a "while" statement in the unlock method. I felt that an "if" was OK, since the object we were checking could not change. Max felt (again this is from memory) that all such checks should be in a while loop.
Since you are storing "this" instance of the Data class in your lock, you are correct to have a separate synchronized block on the static map. You cannot synchronize on the entire method.
Regards, Andrew
[ September 11, 2003: Message edited by: Andrew Monkhouse ]

[xi ruo]

Hi Andrew
About the saying I quoted from you,the thread following this link:
source of the quotation

Since you are storing "this" instance of the Data class in your lock, you are correct to have a separate synchronized block on the static map. You cannot synchronize on the entire method.

In other words,you agree to separate synchronized block on the static HashMap?So,My coding like this is OK?
[ September 11, 2003: Message edited by: xi ruo ]

[Andrew Monkhouse]

Hi Xi,
Your link points back to this page.

In other words,you agree to separate synchronized block on the static HashMap?So,My coding like this is OK?

Yes.
Regards, Andrew

[xi ruo]

Hi Vlad,
Now I have another idea to book a selected record instead of directly coding in the suncertify.gui.GUIController class.
I add a extra method to AnotherDBMain interface which is implemented by DataAdapter,this method definition following:

pakcage suncerty.db;
import java.rmi.RemoteException;
public interface AnotherDBMain {
......
public void book(int recNo) throws RemoteException,RecordNotFoundException;
......
}

In my DataAdapter class

package suncertify.db;
import java.rmi.RemoteException;
public class DataAdapter implements AnotherDBMain {
Data db=new Data(datafilepath);
......
public void book(int recNo) throws RemoteException,RecordNotFoundException {
db.lock(recNo);
db.read(recNo);
db.update(recNo);
db.unlock(recNo);
}
......
}

so,this locking chain like this:lock-read-update-unlock
Please comment.
[ September 12, 2003: Message edited by: xi ruo ]

[xi ruo]

sorry,Andrew,
source of the quotation

[Andrew Monkhouse]

Hi Xi
Wow - I am impressed you read all the way through that topic.
I assume that the code you posted was just to give an indication of what you intend to do - it is nowhere near complete code.
As such, you have got the basic calls correct, now all you need is to flesh it out.
Regards, Andrew

[xi ruo]

Hi Andrew,

I assume that the code you posted was just to give an indication of what you intend to do - it is nowhere near complete code.
As such, you have got the basic calls correct, now all you need is to flesh it out.

Yes,this is only my notion and unimplemented.
To your opinion,Is it viable?Another question is that which end is the implementation,server or client?
[ September 12, 2003: Message edited by: xi ruo ]

[Vlad Rabkin]

Hi Xi,

so,this locking chain like this:lock-read-update-unlock

Yes. It looks now much better!
Best,
Vlad

[Arjan Broer]

I was mainly concerned about the use of Thread.currentThread(). The RMI specifcation does not garantee that a client will always use the same server thread.
So is currentThread a proper identification of the client holding the lock?

[Vlad Rabkin]

Hi Arjan,

So is currentThread a proper identification of the client holding the lock

.
It is Ok for Socket Network Implementation, but If your implementation is based on RMI or you want your implementation be copatibale with RMI, the answer is NO!.
Best,
Vlad

[Andrew Monkhouse]

Hi Arjun
As Vlad pointed out, you cannot use the thread as your identifier in RMI.
However Xi was only using it for reporting purposes (which is also invalid, but he will hopefully be pulling those System.out statements out of his code before submission).
Xi was actually using the instance of the Data class as his unique identifier. This is reasonable.
Regards, Andrew