make server threadsafe. How??? please help!

[jiaquan sun]

HI,
it's mentioned that we must make server and db classes threadsafe. How to do it?
My server just create an instance of a RemoteData object (which extends UnicastRemoteObject and redirects every mehtod call to a single Data instance),Then bind it to the RMI-registry.
and I don't use "snchronized" keyword in the RemoteData object.because methods in the Data class is already snchronized.
I think my server is thread safe.is it?
Please help me clarify this? Thanks!
and,sorry for my english.

[John Smith]

My server just create an instance of a RemoteData object (which extends UnicastRemoteObject and redirects every mehtod call to a single Data instance),Then bind it to the RMI-registry.
and I don't use "snchronized" keyword in the RemoteData object.because methods in the Data class is already snchronized.

The "thread safety" term in the context of this assignment refers to multiple clients trying to book the same flight. To accomplish this, you need to implement some locking mechanism, which you didn't describe in your post.
Eugene.

[Jawad Kakar]

I have a class called LockManger which does the locking and unlocking of records. Moreover lockManger is a singleton (because we need only one lock manager instance, I hope Peter does not have any problem with LockManager as a singleton
).

 
 private static Map lockedRecordes = new HashMap();
  private LockManager(){}
// this method is called in RemotedataServer's lock method, to get single instance of LockManager
public static LockManager getLockManagerInstance(){
    if(lockManagerInstance == null){
      lockManagerInstance = new LockManager();
    }
    return lockManagerInstance;
  }
// THis is my lock() method
public synchronized void lock(RemotedataServer owner, int recordNum){
  try {
    Integer recordKey = new Integer(recordNum);
//    lockedRecordes.put(new Integer(-1), owner);
    while (lockedRecordes.containsKey(recordKey) || lockedRecordes.containsKey(new Integer(-1))) {
      System.out.println("Client:  "+ owner.hashCode() +" Waiting ... To Get The Lock On Record "+ recordKey.toString()+" Size of lockManager is "+lockedRecordes.size());
      this.wait();
    }
    lockedRecordes.put(recordKey, owner);
    System.out.println("Client:  "+ owner.hashCode() +" Got The Lock On Record "+ recordKey.toString()+" Size of lockManager is "+lockedRecordes.size());
  }
  catch (InterruptedException e) {
    e.printStackTrace();
  }
}
// this is how I unlock a given record.
public synchronized void unlock(RemotedataServer owner, int recordNum){
  Integer recordKey = new Integer(recordNum);
  if(lockedRecordes.containsKey(recordKey) && lockedRecordes.containsValue(owner)){
    lockedRecordes.remove(recordKey);
    System.out.println("Client:  "+ owner.hashCode() +" Released The Lock On Record "+ recordKey.toString()+" Size of lockManager is "+lockedRecordes.size());
    this.notifyAll();
  }
}
 

Thank you
Jawad

[Peter den Haan]

Originally posted by Jawad Kakar:
[... LockManager is a Singleton] because we need only one lock manager instance. I hope Peter does not have any problem with LockManager as a singleton [...]

You guessed it, he does
Singleton is a creational pattern; by using it, you hardwire the "one-instance-only" constraint deeply into your architecture. The fact that you happen to need only one instance in this project is no justification. A valid justification for a Singleton has to be a fundamental, inherent reason why you can have only one lock instance. It gets worse. Your Singleton is harmful here. How many database-driven applications do you know that can make do with a single database table? How many do you know that need two or more? What does this imply for the reusability of your server classes? And what on Earth did you gain for sacrificing reusability like that? Another buzzword to add to your documentation?
I've said it before, I'll say it again, Singleton counts without a doubt as the most abused pattern around. Use it only if there is a deep, fundamental, architectural reason why you can allow only one instance of a class, and steer well clear otherwise.
Back to your regular scheduled programme---
In the code above, the fact that you'd never want to replace the lockedRecordes map can be made explicit by making it final. Your getLockManagerInstance() method is not threadsafe; a race condition can lead to the creation of two or more lock managers and thereby to double-booking seats. Synchronize the method (it will synchronize on LockManager.class rather than a LockManager instance, so it won't compete for locks with the instance methods). In lock(), you are creating lots of new Integer(-1) objects; move this into a private static final field

private static final Integer DATABASE_LOCK = new Integer(-1);

The unlock() method does not work correctly if a client owns more than one lock; like the Singleton, this is another arbitrary restriction that happens to be okay for Fly By Night but otherwise severely restricts the reusability of the class without making your life any easier.
Database locks aren't really handled; I assume this is a work in progress?
- Peter

[Jawad Kakar]

From GOF

It�s important for some classes to have exactly one instance. Although there can be many printers in a system, there should be only one printer spooler. There should be only one file system and one window manager. A digital filter will have one A/D converter. An accounting System will be dedicated to serving one company.

I could conclude from above, there should be only one LockManager for FBN application. One and only one central place where all clients can lock their records.

the fact that you'd never want to replace the lockedRecordes map can be made explicit by making it final

Made lockedRecordes map final.

Your getLockManagerInstance() method is not threadsafe

Made it thradsafe.

In lock(), you are creating lots of new Integer(-1) objects; move this into a private static final field

Made the change.

The unlock() method does not work correctly if a client owns more than one lock; like the Singleton, this is another arbitrary restriction that happens to be okay for Fly By Night but otherwise severely restricts the reusability of the class without making your life any easier

Test it with one client holding 23 recordes, worked fine, Could you explain this in more detail please.

Database locks aren't really handled;

I guess I did handle Database lock in lock() method by doing

  
while (lockedRecordes.containsKey(recordKey) || lockedRecordes.containsKey(DATABASE_LOCK)) {
        System.out.println("Client:  "+ owner.hashCode() +" Waiting ... To Get The Lock On Record "+ recordKey.toString()+" Size of lockManager is "+lockedRecordes.size());
        this.wait();
      }

if (-1) is in lockedRecordes all clients waits. Is that right or I have to do something different to handle Database lock.
Peter, I scan almost all the posting and if you have replied to a posting I read it carefully, I have learned lots from you so far.
Thank you for mentioning the weakness in my code.
Jawad
Jawad

[John Lee]

I think RMI model is thread safe. That is also one of the advantages of RMI. Other than this, lock/unlock also will provide thread safe to db class.

[Aruna Raghavan]

Hi All,
I just started to work on this assignment. The database provided is a file. So, doesn't the file need to be locked every time some thing is written to it? I don't see where in the Data class it keeps track of record objects. I am confused about how locking an individual record would help when the object that is holding the data that needs to be protected is in a file?
Thanks in advance for any help.
Aruna.

[John Lee]

quote:
--------------------------------------------------------------------------------
Originally posted by Aruna Raghavan:
doesn't the file need to be locked every time some thing is written to it?
--------------------------------------------------------------------------------
Definitely.

quote:
--------------------------------------------------------------------------------
Originally posted by Aruna Raghavan:
I am confused about how locking an individual record would help when the object that is holding the data that needs to be protected is in a file?
--------------------------------------------------------------------------------
The protection for db file is achieved by synchronization on physical read and write process, which has been taken care of. Lock/unlock provide a protection to business logic:
You can order a flight ticket you see on your screen.
--------------------

[Peter den Haan]

Originally posted by Jawad Kakar:
From GOF [...] I could conclude from above, there should be only one LockManager for FBN application. One and only one central place where all clients can lock their records.

Ah, but is that true?
It isn't. There is no fundamental reason why an application (not FBN, obviously -- but remember that the database code is reusable!) cannot have more than one table. In fact, there are many, many reasons to expect applications to have more than one. Heck, even the FBN system will use multiple tables, even though the little bit that you are doing in the assignment uses only one of them.
Examine your examples from GoF again. They are the kind of fundamental reasons I talked about. If multiple print spoolers would exist, prioritisation and queuing would go haywire; the singleton pattern expresses an important constraint of the very concept of the printer spool. Etcetera. The nature of the GoF examples is markedly different from the single table in FBN. That the assignment uses one table is sheer coincidence; it could just as easily be two or three or a hundred.
Now there is a fundamental constraint in this system: there should be exactly one lock manager per table (i.e. per Data instance). If you'd encapsulate that "per-table singleton" pattern in your design, that would be entirely justified.

Test [the unlock() method] with one client holding 23 recordes, worked fine, Could you explain this in more detail please.

Your unlock() method tests that the map contains the given record lock, and it tests that the lock map contains some lock owned by the client. It does not, however, test that the given record lock is actually owned by that client. This is a bug that would allow a client to unlock a record lock held by another client, thereby violating the unlock() javadoc.

guess I did handle Database lock in lock() method by doing [...]

All your code does is ensure that no new locks are granted while a database lock is held. A lock(-1) call will actually return successfully even if there are still record locks held by other clients; surely that is wrong. Beware of potential deadlock issues when implementing this. Depending on your interpretation of the database lock requirement you may also want to implement unlock() functionality.
- Peter
[ December 26, 2002: Message edited by: Peter den Haan ]

[Jawad Kakar]

Peter,
Thank you for your reply,

 public synchronized void unlock(RemotedataServer owner, int recordNum){
    Integer recordKey = new Integer(recordNum);
   [B] if(lockedRecordes.containsKey(recordKey) && lockedRecordes.containsValue(owner))[/B]{
      lockedRecordes.remove(recordKey);
      System.out.println("Client:  "+ owner.hashCode() +" Released The Lock On Record "+ recordKey.toString()+" Size of lockManager is "+lockedRecordes.size());
      this.notifyAll();
    }
  }

Moreover I test lock/unlock creating 500 threads in batch of 10 using the following test program and it seems to me if client 5 locks record 10, then only client 5 unlocks record 10 before any other client try to lock records 10.
I guess I am missing something could you please tell me how to change unlock to fix it.
Test program

  
public void LockingAndUnlockingTest(final int recNum){
    Thread th = new Thread(){
     public void run(){
       try{
         // returns a distinct remote connection for each client.
         DataClient dc = suncertify.client.ConnectionFactory.getRemoteConnection(StartupDialog.getController());
         //lock
         dc.lock(recNum);// calls the lock() from LockManager ie lock(recNum){lockManagerInstance.lock(recNum)}
         // unlock
         dc.unlock(recNum);// calls the unlock() form LockManger ie lock(recNum){lockManagerInstance.unlock(recNum)}
       }
       catch(Exception e){
         System.out.println(e.getMessage());
         e.printStackTrace();
       }
     }
   };
   th.start();
  }

Thank you
Jawad
[ December 26, 2002: Message edited by: Jawad Kakar ]

[Peter den Haan]

If client1 locks record 5, and client2 locks record 7, then client1 can successfully unlock record 7 which it doesn't own. Try it.
The problem is your containsValue() call which tests whether the owner exists anywhere in the lock map. This is not strict enough; you should test whether the particular lock at hand is owned by the client.
- Peter

[Jawad Kakar]

Peter,
The situation that you have mentioned never happened in my test, but I see the problem in code clearly, thank you very much for the help and I have changed the unlock method to

  
public synchronized void unlock(RemotedataServer owner, int recordNum){
      Integer recordKey = new Integer(recordNum);
      // returns the value to which lockedRecordes map maps recordKey  key.
      RemotedataServer rs = (RemotedataServer)lockedRecordes.get(recordKey);
      if(rs == owner){
        lockedRecordes.remove(recordKey);
        System.out.println("Client:  "+ owner.hashCode() +" Released The Lock On Record "+ recordKey.toString()+" Size of lockManager is "+lockedRecordes.size());
        this.notifyAll();
      }
  }

Thank you
Jawad

[Peter den Haan]

That looks fine to me. Glad to be of help.
- Peter

[John Smith]

Jawad,
You code actually reduces to this (if you are into perfection):

    synchronized public void unlock(int recNum, Object clientId) {
        Integer record = new Integer(recNum);
        if (lockedRecords.get(record).equals(clientId)) {
            lockedRecords.remove(record);
            notifyAll();
        }
    }

Eugene.

[Jawad Kakar]

Eugene,
Thanks for your reply
Which one is better, passing a RemotedataServer for clientId or passing an Object. As far as reusability is concern passing Object is a better idea, am I correct or there is some other reason for it too.

 
public synchronized void unlock(RemotedataServer owner, int recordNum){
...
}
public synchronized void unlock(Object owner, int recordNum){
...
}
 

Thank you
Jawad
[ December 27, 2002: Message edited by: Jawad Kakar ]

[John Smith]

Which one is better, passing a RemotedataServer for clientId or passing an Object. As far as reusability is concern passing Object is a better idea, am I correct or there is some other reason for it too.

Yes, refering to the interface or a superclass is always better than refering to a concrete implementation class.
Eugene.

[Peter den Haan]

Originally posted by Eugene Kononov:
Yes, refering to the interface or a superclass is always better than refering to a concrete implementation class.

Eugene is right, of course. A LockManager that takes Objects for identification can be re-used in completely different contexts. If it stipulates RemoteDataServer, it is only reusable in the cut-paste-modify sense (i.e. NOT REUSABLE).
In general it is a Good Thing to make as few assumptions as possible in your code. If it works with Object, use that. If you get an ArrayList but you only use methods from the List interface, use List. If you get a List but your method or class works just as well with a Collection, use the latter instead. If it works with a Data instance but only uses methods from the DataInterface, use that. Etcetera.
- Peter
[ December 28, 2002: Message edited by: Peter den Haan ]

[walid aly]

Dera Friend
i hope i did not come after the party was over
1-about lock manager singlton
if we make class data singelton and lockmanager a class vatiable in it , i think i will always have a single instance of singelton
public class Data
{

private LockManager lockManager=new LockManager();
2-i saw 2 signatires for lock and unlock in ur code , one which does not take a client id and a one that takes client id ...
could u plz clarify this to me, i think when using rmi we have to change the signature of the lock method in class data to take clientid ,am i correct?

walid

[Peter den Haan]

Originally posted by walid aly:
1-about lock manager singlton
if we make class data singelton and lockmanager a class vatiable in it , i think i will always have a single instance of singelton

Sure. But what did you gain, except greater complexity?

2-i saw 2 signatires for lock and unlock in ur code , one which does not take a client id and a one that takes client id ...

The one without client id is the signature exposed by Data and RemoteData/Connection/WhateverYouCallIt. The one with client id is the internal one exposed by the lock manager.

i think when using rmi we have to change the signature of the lock method in class data to take clientid ,am i correct?

No, you're not correct. This is one of the more interesting puzzles in the assignment. I'm sure that if you search this forum for the word "connection" you'll get plenty of hints.
- Peter