NX:Client crashed cause deadlock in LockManager

[Kruger Scheitz]

my lock implementation look something like this
public void lock( record, owner )
{
while( dblock ) {
wait();
}
if( record == -1 )
dblock = true;
}
this is what i'm trying to do.
Whenever a request to lock the whole datebase comes in, I set a boolean to true and if there are any currently locked records I call wait(), I don't allow any more records to be locked, and allow the currently locked records to be unlocked before granting a whole database lock.
Now when clients crashed after calling lock() without call unlock(), it will cause the record to be locked for ever. How to fix this deadlock?

[Mark Spritzler]

Look at the Unreferenced Interface. This will allow you to unlock records from a client that has crashed.
Also to note, the only call to lock for locking the entire database will come from the server when it is shutting down. No client should ever call for a lock of the entire database.
Mark

[S. Ganapathy]

Hi Mark,
Is it must to implement Unreferenced Interface for remote objects?
Is it so necessary to implement the interface in our assignemnt(RMI server)?
If WeakHashMap is used to store locks, after sometime, if there is no client reference, an entry in a WeakHashMap will automatically be removed when its key is no longer in ordinary use. Will this WeakHashMap help?
Please help me.
Regards,
Ganapathy.
[ July 01, 2003: Message edited by: S. Ganapathy ]

[Andrew Monkhouse]

Hi Ganapathy
The problem with WeakHashMap is that your code does not get notified when an object is removed.
Consider when a client (Client 'A') attempts to get a lock, and finds that it is already locked, so it calls wait(). The client that does have this record locked (Client 'B') crashes, so sometime later the reference to this client is removed from the WeakHashMap. But what happens to the client currently waiting for the lock (Client 'A')? The reference has been silently removed from the WeakHashMap, so notify() has not been called.
You could write code which will handle the possibility of locks disappearing, and as long as you do so, you should be OK with WeakHashMaps.
This is where I see the advantage of Unreferenced: Your code is explicitly called when an object becomes unreferenced, so you have more chance of running "normal" clean up operations, such as unlocking any locked records (which should be calling notify(), so your waiting clients get woken up). Of course the disadvantage is that unreferenced() can be called multiple times for the one object, so you have to be careful how you write your code so that it does not cause an infinite loop.
Regards, Andrew

[S. Ganapathy]

Hi Andrew,
I am providing locking functionality in LockManager class. This class holds the record number, and returns a cookie value, which are stored in HashMap object. Locking mechanism in my assignment is entirely at the server side.
Any operation in server side, like update record, is a three step process. First lock the record, then update the record and then unlock the record. Everything is done from single method, but it is not an atomic operation.
Do I need to take care of client crashes?
Once client sends the record to the server, and even if client crashes, as server receives the data, it will compete the operation of lock-update-unlock.
Please advice me.
Regards,
Ganapathy.

[Andrew Monkhouse]

Hi Ganapathy

Everything is done from single method, but it is not an atomic operation.

Since it is not atomic, then I think it would be possible for an orphaned lock to be produced. Granted, it would be difficult, but it is possible. The situation could occur if the client crashes while it's remote code is in wait() state. If the remote code does not get notified of lock availability until moments before Java notices that the client is no longer connected, then it might have time to get the lock, then get killed by the JVM.
Regards, Andrew

[S. Ganapathy]

Hi Andrew,
Once the request goes to the server, even if there is wait(), the server method waits, to lock the record, once it locks the record, further processing will be done like update the record. Even if the update fails, lock on record will get released, and client will get notified. If client already crashes, still server completes the operation. Either updates, or throwing exception.
Is there any insecurity in this process? This is my worry.
Regards,
Ganapathy.

[Andrew Monkhouse]

Hi Ganapathy,
The example with the wait state was just an example of how the potential problem I forsee could manifest itself.
The problem is that you said that your method is not atomic. This implies that the following can occur:

your method calls lock()

the JVM switches out your method while it runs another task

your method does any verification necessary

the JVM switches out your method while it runs another task

your method calls update()

the JVM switches out your method while it runs another task

your method calls unlock()
Now, as I said, in most cases this will be fine. But there is the possibility that in one of those instances where the JVM switched out your method, that it decided that the client is dead, and stops your processing.
Regards, Andrew

[Philippe Maquet]

Hi,
Perhaps I invested too much in my LockManager manager class, but I wanted it to guard myself against any lock issue, as those mentioned in a few threads here :
- risk of deadlocks if I let the user book multiple rooms at once as shown in this pseudo-code :
locking multiple records (risk of deadlock)
if ok reading the records
if ok, update them
finally unlock records really locked
- risk of deadlocks if a client crashes (even if multiple-record-locking is not implemented)
So my LockManager class supports :
. deadlock detection with automatic repair
. automatic unlocking on timeout
. automatic unlocking of records locked by dead threads
. reentrant locking
It works fine and I think it was worth.
Regards,
Philippe MAQUET.

[George Fung]

Hi Philippe,
Could you tell me how to do the following stuffs?
. deadlock detection with automatic repair
. automatic unlocking on timeout
. automatic unlocking of records locked by dead threads
. reentrant locking

[Philippe Maquet]

Hi George,
Why not ? :-)
Here is my lockRecord method (pseudo-code) :

see if record is already locked
  if not, get an immediate lock, finished
  if yes, see if lock owner is currentThread
    if yes, finished (REENTRANCE)
    if not, see if lock owner is waiting for another lock
      if not, simply wait for the lock
      if yes, see if the lock the blocking owner is waiting for, is owned by currentThread or not
        if not, simply wait for the lock
        if yes, we enter in DEADLOCK ! :-)
          unlock the record the the blocking owner is waiting for
          throw DeadLockException (RuntimeException)

    public void book(long [] recNos, ... other needed parameters ...) {
        boolean recNotFound = false;
        long [] lockCookies = new long[recNos.length];
        try {
            boolean deadLock;
            outer:
                do {
                    deadLock = false;
                    for (int i = 0; i < recNos.length; i++) {
                        try {
                            lockCookies[i] = data.lockRecord(recNos[i]);
                        }
                        catch (DeadLockException e) {
                            deadLock = true;
                            break;
                            // stay in the outer loop
                            // (interest of reentrance support in the lock
                            // method)
                        }
                        catch (RecordNotFoundException e) {
                            recNotFound = true;
                            break outer;
                        }
                    }
                } while (deadLock);
                if (!recNotFound) {
                    // place here reads + updates
                }
        }
        finally {
            for (int i = 0; i < recNos.length; i++) {
                // to avoid a SecurityException in case of partial lock
                if (!recNotFound || data.isLockOwned(recNos[i])) {
                    data.unlock(recNos[i], lockCookies[i]);
                }
            }
        }
    }

As far as "automatic unlocking on timeout" and
"automatic unlocking of records locked by dead threads" are concerned, it was quite simple to be implemented by a maintenance thread which tests periodically (every second in my implementation) that :
. every lock is owned by a isAlive() thread ;
. every lock is not owned for more then my lockTimeout property,
and in either case unlock those invalid locks.
I hope it was clear. If any of you see an issue, please tell me.
Phil.
[ July 07, 2003: Message edited by: Philippe Maquet ]
[ July 08, 2003: Message edited by: Philippe Maquet ]

[Andrew Monkhouse]

Hi Philippe
I chose to ignore deadlocks in my assignment, as I believed it was out of scope. None of my clients would ever attempt to gain two locks at the same time, so deadlock could not occur with my code. And I documented that decision. (But I did loose four marks in my server, so who knows - it may have been a bad decision :roll: )
Your deadlock prevention will still allow a deadlock if three or more clients are doing locking at once:

client 'A' locks record 1 and attempts to lock record 2

client 'B' locks record 2 and attempts to lock record 3

client 'C' locks record 3 and attempts to lock record 1
Do you document this?

As far as "automatic unlocking on timeout" and "automatic unlocking of records locked by dead threads" are concerned, it was quite simple to be implemented by a maintenance thread which tests periodically (every second in my implementation) that :

every lock is owned by a isAlive() thread ;

every lock is not owned for more then my lockTimeout property,

and in either case unlock those invalid locks.

What do you do if the lockTimeout property is exceeded?
I am not sure about you throwing RuntimeExceptions for DeadLock (and presumably Timeout). I would think that client code should be able to handle both conditions, but since the client does not have to catch a RuntimeException, a client application could die a horrible death if the programmer did not catch your exception.
The solution that I see is that both these exceptions are only relevant in the networked code, and in the networked code you could throw exceptions that are not listed in Sun's interface, so for the networked code you could have some checked exceptions for deadlock and timeout.
Regards, Andrew

[Andrew Monkhouse]

Hi Philippe
Also, I am unsure about your looking at thread death as a way of determining if the client has died. I guess it depends on what thread you are using. Just using the thread running your remote class is not really valid though, as there is no guarantee that two calls to remote methods from the one client will run in the same thread.
Take a look at this code:

import java.util.*;
import java.rmi.*;
import java.rmi.registry.*;
import java.rmi.server.*;
interface ServerRef extends Remote {
    public void doSomething() throws RemoteException;
}
class Server extends UnicastRemoteObject implements ServerRef {
    ThreadWatcher threadWatcher;
    public Server() throws RemoteException { 
        threadWatcher = new ThreadWatcher();
        threadWatcher.start();
    }
    public void doSomething() throws RemoteException {
        System.out.print("server " + Thread.currentThread().hashCode());
        System.out.println(" started " + new Date());
        threadWatcher.addThread(Thread.currentThread());
    }
}
class ThreadWatcher extends Thread {
    Thread threadsToWatch[] = new Thread[5];
    int numThreads = 0;
    public void run() {
        System.out.println("Thread watcher started");
        while (true) {
            for (int i = 0; i < numThreads; i++) {
                if (threadsToWatch[i] != null && ! threadsToWatch[i].isAlive()) {
                    System.out.print("Thread " +  threadsToWatch[i].hashCode());
                    System.out.println(" died " + new Date());
                    threadsToWatch[i] = null;
                }
                try {
                    Thread.sleep(1000);
                } catch (InterruptedException ignored) {}
            }
        }
    }
    public void addThread(Thread thread) {
       threadsToWatch[numThreads++] = thread;
    }
}
public class RmiTimeout {
    public static void main(String[] args) throws Exception {
        try {
            startServer();
        } catch (Exception e) {
            runClient(args);
        }
    }
    private static void startServer() throws Exception {
        // System.setProperty("java.rmi.dgc.leaseValue", "30000");
        // System.setProperty("sun.rmi.transport.connectionTimeout", "10000");
        LocateRegistry.createRegistry(1234);
        Naming.rebind("rmi://localhost:1234/RmiTimeout", new Server());
        System.out.println("Server ready");
    }
    public synchronized static void runClient(String[] args) throws Exception {
        ServerRef remoteCode =
                (ServerRef) Naming.lookup("rmi://localhost:1234/RmiTimeout");
        remoteCode.doSomething();
        System.out.print("Sleeping for ");
        for (int i = 35; i > 0; i--) {
            System.out.print(i + ", ");
            Thread.sleep(1000);
        }
        System.out.println("0");
        System.out.println("Should have a different thread on server now");
        remoteCode.doSomething();
    }
}

Running it gives me the following output:

[andrewm@andrewm a]$ java RmiTimeout
Thread watcher started
Server ready
server 10923757 started Fri Jul 04 11:36:32 EST 2003
Thread 10923757 died Fri Jul 04 11:37:03 EST 2003
server 13170424 started Fri Jul 04 11:37:08 EST 2003
Thread 13170424 died Fri Jul 04 11:37:09 EST 2003

Even though the client still had a valid connection, I still got notified of the first thread death.
(I had been playing with setting the lease value, but doing this seemed to change how long the thread stayed active for (both calls to the remote method ran in the same thread, even with a 60 second delay) so I commented out that code).
Regards, Andrew

[Philippe Maquet]

HI Andrew,

I chose to ignore deadlocks in my assignment, as I believed it was out of scope. None of my clients would ever attempt to gain two locks at the same time, so deadlock could not occur with my code.

Whichever point of view I take, I believe that deadlock prevention is a must :
As the URLyBird application writer :
----------------------------------
The specs say that "the user interface must allow the user to book a selected record, updating the database file accordingly."
What does mean "a" (selected record) ? Does it mean "one" or "a" among a few ?
I think that booking hotel rooms is typically a transactional activity : if you - as a customer - try to organize a 3 days-2 nights city-trip, you'll want to book either 2 rooms or none.
So I'll let the user multi-select records to book them all in a single transaction.
As the Data class writer :
------------------------
I claim that I implement the DBAccess interface, which in turn allows locking multiple records by a same thread. I cannot make any assumption about how other programmers (perhaps in other applications) will use it. Simply documenting that deadlock situations are not handled is not enough, because it's like you write something like "Warning : using the Data class as described in its interface could lead your application to hang indefinitely." :-)
Another approach could be to keep deadlock prevention as a further improvement and in the meantime prevent any thread to own more than one lock. I had 2 reasons not to do so (the first is good, the second very bad) :
1� As when you build a house, some features are difficult to add later, if they relate to the foundations, and that's the case here.
2� It was so exciting and fun to design and code it ! :-)

Your deadlock prevention will still allow a deadlock if three or more clients are doing locking at once:
client 'A' locks record 1 and attempts to lock record 2
client 'B' locks record 2 and attempts to lock record 3
client 'C' locks record 3 and attempts to lock record 1

Well, thank you *very* *very* much for pointing this out. I will not document it because I hate to document bugs, I prefer to correct them ! :-)
If such locking situation arise in my current code, the deadlock will not be prevented but well broken by my maintenanceThread when the blocking lock times out. It means that the application will not hang, but it's not acceptable for all that.
Here is the situation you describe :

ABC
123
2w3w?

When C wants to get a lock on record 1 which is not available immediately, instead of asking to himself :
"Am I the owner of the lock the blocking thread is waiting for ?",
it must ask the following question :
"Am I the owner of the lock the blocking threads chain is waiting for ?"
It's easy to correct by recursivity :
C wants 1 locked by A waiting for 2 locked by B waiting for 3 locked by ... me >> deadlock
Thanks again !

What do you do if the lockTimeout property is exceeded?

The maintenanceThread unlocks the record. Of course, there is no exception thrown, because there would be nobody to catch and handle it (except the JVM ! :-)).
In fact, you get the same situation as if the lock had never been granted. It means that when the thread who "thinks" owning the lock will try to use it, he will receive a SecurityException. But he can prevent it by testing its lock with the boolen isLockOwned(recNo) method.

I am not sure about you throwing RuntimeExceptions for DeadLock (and presumably Timeout). I would think that client code should be able to handle both conditions, but since the client does not have to catch a RuntimeException, a client application could die a horrible death if the programmer did not catch your exception.

As I explained above, I have no TimeoutException.
When LockManager detects a deadlock, it's common sense that he must communicate it to the caller. As DBAccess interface cannot be changed (no new checked exceptions are allowed), I see only 3 ways to do it :
1� From the long lockRecord(long recNo), return a special cookie value (0 by example) : it'd been my preferred solution in 1986 when I was programming in Clipper... :-)
2� Use the new exception chaining facility : wrap my DeadLockException as the cause of the checked RecordNotFoundException, and throw the latter instead. I feel that's as stupid as solution 1�.
3� Throw my DeadLockException (runtime) and document it with the @throws javadoc tag. I don't like it, but it's still better than 1� and 2�.
I am a Java newbie. So if you think that I am wrong, please be kind to let me know.

Also, I am unsure about your looking at thread death as a way of determining if the client has died. I guess it depends on what thread you are using. Just using the thread running your remote class is not really valid though, as there is no guarantee that two calls to remote methods from the one client will run in the same thread.

I use sockets and make sure that a client connection keeps the same thread. But of course, I will document that locks cannot spread over multiple spread, and explain why.
Thanks again for your comments. They were helpful !
Regards,
Phil.
[ July 04, 2003: Message edited by: Philippe MAQUET ]

[Andrew Monkhouse]

Hi Philippe
I like your reasoning for the deadlock prevention. I have used many of the same arguments to explain why I believe that the client should be able to call the lock() and unlock() methods directly, rather than calling a modify() method on the server.

When LockManager detects a deadlock, it's common sense that he must communicate it to the caller. As DBAccess interface cannot be changed (no new checked exceptions are allowed), I see only 3 ways to do it :

I agree that option 1 is not OO, and option 2 is just plain ugly. Which leaves option 3.
Just to give you another option to think about though. I think you are coding such that:

ClientConnectivity --> Data --> LockManager

This way round you are correct - you are limited by the exceptions that may be thrown by the Data class.
Turn it around though, and you loose that restriction:

ClientConnectivity --> LockManager  --> Data

This has other issues though. And the one that I think you will hate is that this means that anyone accessing the Data class directly will loose all the work you have put into the LockManager.
Regards, Andrew
[ July 04, 2003: Message edited by: Andrew Monkhouse ] (I got caught by that UBB bug where editing a post with quotes goes crazy!)
[ July 04, 2003: Message edited by: Andrew Monkhouse ]

[Philippe Maquet]

Hi Andrew,

I have used many of the same arguments to explain why I believe that the client should be able to call the lock() and unlock() methods directly, rather than calling a modify() method on the server.

Nothing in my assignment prevents neither force me to expose the Data lock/unlock methods to the client, but I decided not to do so after weighting pros and cons :
pros : I don't see any :-)
cons : scattering "functionally atomic" operations (such as booking rooms) among multiple network connections is

less reliable

less efficient

This has other issues though. And the one that I think you will hate is that this means that anyone accessing the Data class directly will loose all the work you have put into the LockManager.

You're right, taking into account my global design decisions as far as data access is concerned :

My Data class is a multiton, in order to be sure that no more than one instance per database file exists in a single JVM. The only way to get a Data instance is to use its static method Data getInstance(String dataFileName).

LockManager is not public and is a multiton too. That design allows locks to be managed (including deadlock prevention) accross multiple table, which is classic. Each Data instance gets its LockManager reference by calling the static LockManager getInstance(Data data).

I hope the whole stuff seems consistent.
Regards,
Philippe.

[Vlad Rabkin]

Hallo Andrew,
Hallo Philippe,
Could you help me understand the ideas about Unreferenced or WeakHashMap.
Let's take WeakHashMap:
My key is Record Number (Wrapped int).
It is my constructor of Data:
public Data(String fileName, LockManager lockManager)
My C-tor for RemoteData:
public DataRemote(DB db, ReadWriteLock rwManager)
In DataRemote in lock method I do have following:
cookie = db_.lock(recNo);
and in unlock:
db_.unlock(recNo, cookie);
, where recNo is int.
So, actually my DataRemote has no direct reference on Key (Integer(recNo)).
It has only reference on Data.
If I do WeakHashMap, lock will always suddenly dissapear! ??? Is it right? If so, how should I do it?
Thanx,
Vlad

[Philippe Maquet]

Hi Vlad,
Unfortunately, I cannot be of much help currently as far as Unreferenced and WeakHashMap are concerned :

Unreferenced is a RMI topic and as I chose sockets, I didn't dig into RMI that much.

I am not a WeakHashMap specialist, as that basic question (still pending BTW) shows in : NX: URLyBird (Database Schema Parser)

Maybe Andrew ...
Cheers,
Phil.

[Vlad Rabkin]

This question in the topic pointed by U is irrelevant for me, because my Data is Singleton.
Anyway, thanx!

[Max Habibi]

Originally posted by Vlad Rabkin:
Hallo Andrew,
Hallo Philippe,
Could you help me understand the ideas about Unreferenced or WeakHashMap.
Let's take WeakHashMap:
My key is Record Number (Wrapped int).
It is my constructor of Data:
public Data(String fileName, LockManager lockManager)
My C-tor for RemoteData:
public DataRemote(DB db, ReadWriteLock rwManager)
In DataRemote in lock method I do have following:
cookie = db_.lock(recNo);
and in unlock:
db_.unlock(recNo, cookie);
, where recNo is int.
So, actually my DataRemote has no direct reference on Key (Integer(recNo)).
It has only reference on Data.
If I do WeakHashMap, lock will always suddenly dissapear! ??? Is it right? If so, how should I do it?
Thanx,
Vlad

Almost correct. In the WeakHashMap, your key is the reference to the client, and your value is the Integer wrapped recno. That way, when the client crashes, RMI will eventually release that client. When they are released, then the WeakHashmap will release the record.
Alternately, you could implement Unreferenced. That way, when the client crashes, the Unreferenced method will be called, In this scenario, you can write code that explicitly release the lock.
Both are valid solutions, and both offer challenges. The problem with un referenced, IMO, is twofold. First, it's more complicated(you have to write extra code: WeakHashMap doesn't require any code at all). Second, it's possible that Unreferenced will be called several times, because the reference count can drop to zero, then back up again, then back to zero. Unreferenced, of course, is called whenever the reference count hits zero, not just the first time.
WekaHashMap also has a theoretical problem, because notifyAll could never be called on the map when the client is released. Thus, if you have a very high number of clients, it's possible that the normal scheduling happens too quickly for a new thread to pick up on the fact that the map is free. I have never been able to produce this behavior, but it's possible. Either way, it's a toss up. IMO, WeakHashMap is a little better, but other reasonable opinions differ.
All best,
M

[Vlad Rabkin]

Hi Max,

In the WeakHashMap, your key is the reference to the client, and your value is the Integer wrapped recno.

WAW, I don't understand it ...
Where do U keep ypur cookies then?
My Key is ID(Integer) and my Value is Lock object (This class wrappes only the long cookie).
And what you mean id is client? What class/object do U mean?
Thanx,
Vlad

[Max Habibi]

Originally posted by Vlad Rabkin:
Hi Max,


WAW, I don't understand it ...
Where do U keep ypur cookies then?

In a box under the bed, so my wife doesn't throw them away .

My Key is ID(Integer) and my Value is Lock object (This class wrappes only the long cookie).
And what you mean id is client? What class/object do U mean?
Thanx,
Vlad

Let me back up a bit: Can you lay out your design in a bit more detail?
M

[Vlad Rabkin]

Hi Max,

Let me back up a bit: Can you lay out your design in a bit more detail?

So, just have changed the design a bit:
I use RMI.
I have a Factory (is published in RMI Registry) which delivers one remote object pro Client (So, it is not a Singelton). The main method of it creates LockManager(Singelton), Data(Singelton)and ReadWriteLock(Singelton)
Every time getConnection() on Factory from client is called a new DataRemote Object is created and it gets (via C-tor) LockManager object, Data object and ReadWriteLock object.
Now, Lockmanager has internally synchronized HashMap. This HashMap holds RecordNumbers (int) (wrapped in Integer) as its Keys and cookies (wrapped in a my own simple class called Lock).
LockManager provides 3 methods:

// waits if such is already 
long lock(int recNo); in HashMap
// removes element from Map and notifies Waiters
void unlock(int recNo, long cookie);
void checkAccess(int recNo, long cookie)
throws SecurityException;
(all of them are synchronized)  

That's it.
So, the lock have nothing directly to do with clients or RemoteData object, they are dealing only with "client threads" on the server and Record Numbers, which are locked.
I hope I shouldn't completely reconsider my design to us Unreferenced or WeakHashMap
Thanks you much in advance...
Vlad

[Max Habibi]

Vlad,
Given you design, I think that either Unreferenced or a WeakHashMap could help you, and fairly easily @ that. For WeakHashMap, what you want, I think, is to pass in the recno from the unique remote client to the LockManager, who'll just hold on to it. It's important that the Integer(recNo) actually be an Integer that's created by the unique remote RMI object, and then passed in as a paramater. Why? Because when that remote object goes out of scope, it's member variables will be gc'd. When that happens, the recno be be removed, releasing the int.
So, basically,
1. use a Weakhashmap instead of a HashMap,
2. Make sure that your lock take an Integer as a pram, instead of taking an int and wrapping i on the fly.
HTH

[Vlad Rabkin]

Ok, Max
Thanx a lot!
Vlad

[Jim Yingst]

Mmmm, I don't think the WeakHashMap is as clean a fit here as Max has implies.
2. Make sure that your lock take an Integer as a pram, instead of taking an int and wrapping i on the fly.
This is a problem - the lock() method defined in the API we must implement required an int, not Integer. We can't change that. I suppose we could provide an overloaded lock() method that takes an Integer and is not part of the DB interface (called DBMain in some assignments, I think). But that feels wrong to me, to have a method in the DB interface that is almost what we need, and then we don't use it at all, instead relying on another method. If we're going to do that, why not give Data some additional methods that just omit cookies entirely? After all, they just get in the way of a clear, simple program.
Seriously, my Data does provide some additional methods not in DB - notably getMetaData() which allows me to avoid hardcoding column names and lengths. But I try to use the previously specified DB methods wherever possible, which in this case means using lock(int) rather than lock(Integer). So this solution seems unsatifactory to me. Furthermore I worry about those pesky junior programmers, who may not really understand why they must use lock(Integer) rather than lock(int). (And I hope they don't decide to use that same Integer anywhere else - though that seems unlikely.) I'm thinking the WeakHashMap approach makes more sense for people doing an assignment without cookies (there are some like that, right?).
My own solution is: A ConnectionFactory returns Connection instances, one per client connection. Connection is an interface whose server-side implementation is ConnectionImpl. Each ConnectionImpl carries its own HashMap or the record numbers and cookies pertaining to the locks held by that particular connection[/b]. Connection has a close() method which iterates though the map and closes all associated connections. This close() method can be called by both unreferenced() [i]and finalize(). I may not really need both, but why not; it can't hurt. The close() method is written in such a way that it doesn't matter if it's called more than once.
It does feel a bit cumbersome for each Connection to have its own HashMap. They were previously very lightweight objects; now they're a bit heavier. If we had a lot of clients and were worrying about memory usage and performance, this could be a problem. But there's little justification for that particular concern now, IMO. In the real world, I'd advise the customer to just get rid of those dame cookies, and the problem will go away entirely.
Both are valid solutions, and both offer challenges. The problem with un referenced, IMO, is twofold. First, it's more complicated(you have to write extra code: WeakHashMap doesn't require any code at all).
A valid concern in general. Though I think there's a certain advantage in having cleanup code appear explicily in our classes rather than being handled behind the scenes. With explicit code, junior programmers will at least know that it's there. It's isolated in one class; easy enough to skip past unless/until it's necessary to modify the cleanup somehow.
Second, it's possible that Unreferenced will be called several times, because the reference count can drop to zero, then back up again, then back to zero. Unreferenced, of course, is called whenever the reference count hits zero, not just the first time.
Interesting point. I'm thinking this is a non-issue for my code, but want to check my understanding. How would the reference count increase after it's hit zero? The main way that comes to mind is if the instance is bound to the registry - any clint that does Naming.lookup() with the appropriate name will get a new cleint reference to the object, no matter how many other client refereces exist. So OK, beware of making a bound object Unreferenced, as the unreferenced() method may be called multiple times. But with the ConnectionFactory, only the factory is bound. Each Connection instance returned by the factory's connect() method is a brand new instance, and only one client reference exists - from the client who did the connect(). In this context, a call to unreferenced() seems pretty unambiguous - that client is done, period, and won't be called again. Does that make sense? Is there some other mechanism through which the count of remote references could increase from zero? My experience with RMI is pretty limited, so I could well be overlooking something.

[Max Habibi]

/*

Originally posted by Jim Yingst:
Mmmm, I don't think the WeakHashMap is as clean a fit here as Max has implies.

It has it's own problems, to be sure: nothing is prefect. One of these is that fact the even as an element is removed, the map might not get notifyAll called on it(as stated above, I think). However, this has nothing to do with a weakHashMap per se: it's just the nature of anything that takes advantage of both synchronization and Unreferenced at the same time. Your own code is limited by this. That is, if your Unreferenced code isn't synchronized on the hashmap, then it won't notify the map when you quitely remove an element. OTOH, if it is synchronized on the Map, it might never get called, because a client that's holding on the lock might die without ever calling notifyAll().

2. Make sure that your lock take an Integer as a pram, instead of taking an int and wrapping i on the fly.

This is a problem - the lock() method defined in the API we must implement required an int, not Integer. We can't change that.

Nor should we: However, that's not the map Vlad and I were talking about. Vlad isn't talking about Data, he's talking about his lock manager.

Now, Lockmanager has internally synchronized HashMap.

I suppose we could provide an overloaded lock() method that takes an Integer and is not part of the DB interface (called DBMain in some assignments, I think). But that feels wrong to me, to have a method in the DB interface that is almost what we need, and then we don't use it at all, instead relying on another method. If we're going to do that, why not give Data some additional methods that just omit cookies entirely? After all, they just get in the way of a clear, simple program.
Seriously, my Data does provide some additional methods not in DB - notably getMetaData() which allows me to avoid hardcoding column names and lengths. But I try to use the previously specified DB methods wherever possible, which in this case means using lock(int) rather than lock(Integer). So this solution seems unsatisfactory to me. Furthermore I worry about those pesky junior programmers, who may not really understand why they must use lock(Integer) rather than lock(int). (And I hope they don't decide to use that same Integer anywhere else - though that seems unlikely.) I'm thinking the WeakHashMap approach makes more sense for people doing an assignment without cookies (there are some like that, right?).
My own solution is: A ConnectionFactory returns Connection instances, one per client connection. Connection is an interface whose server-side implementation is ConnectionImpl. Each ConnectionImpl carries its own HashMap or the record numbers and cookies pertaining to the locks held by that particular connection[/b]. Connection has a close() method which iterates though the map and closes all associated connections. This close() method can be called by both unreferenced() [i]and finalize(). I may not really need both, but why not; it can't hurt. The close() method is written in such a way that it doesn't matter if it's called more than once.

This is where the Weakhashmap could save you a lot of trouble, I think. I like the ConnectionFactory: it's makes a lot ofsense. But. Simple have all of those remote connections share the same WeahHashMap(instead of a private HashMap), and presto, you're done. The extra code you're writing for Unreferenced, as well the code guaranting that the close() method is safe if called called twice, as well the finalize call all go away. You don't need them at all anymore. Thus, no more complexity then necessary, but exactly as much functionality. A given client will have an instance of Data as a member variable. Data, in turn, will have the static WeakHashMap.

It does feel a bit cumbersome for each Connection to have its own HashMap.

I don't think I understand what you're doing here. Can you elaborate a bit about what's in these maps?

They were previously very lightweight objects; now they're a bit heavier. If we had a lot of clients and were worrying about memory usage and performance, this could be a problem. But there's little justification for that particular concern now, IMO. In the real world, I'd advise the customer to just get rid of those dame cookies, and the problem will go away entirely.
Both are valid solutions, and both offer challenges. The problem with un referenced, IMO, is twofold. First, it's more complicated (you have to write extra code: WeakHashMap doesn't require any code at all).
A valid concern in general. Though I think there's a certain advantage in having cleanup code appear explicitly in our classes rather than being handled behind the scenes. With explicit code, junior programmers will at least know that it's there. It's isolated in one class; easy enough to skip past unless/until it's necessary to modify the cleanup somehow.

There's stuff going behind the scenes anyway: the gc, the dgc, the callbacks for Unreferenced, etc. IMO, and again, only MHO, the weakHashMap is a discreet bundle of isolation, written by Sun, that does everything we need here. I'm not sure I'm motivated to duplicate it's functionality.

Second, it's possible that Unreferenced will be called several times, because the reference count can drop to zero, then back up again, then back to zero. Unreferenced, of course, is called whenever the reference count hits zero, not just the first time.
Interesting point. I'm thinking this is a non-issue for my code, but want to check my understanding. How would the reference count increase after it's hit zero? The main way that comes to mind is if the instance is bound to the registry - any clint that does Naming.lookup() with the appropriate name will get a new cleint reference to the object, no matter how many other client refereces exist.

RMI may decide to recycle the object to another client. Try this. Write 2 remote threads, and give the object to one of them. Then, after the client releases the object, ref count hits zero. Now, give the object to the second client. When that second thread is finished(caput, dead, no longer with us, visiting Elvis), the ref count will again hit zero, and Unreferenced will again be called. The problem here is that you have to write fairly non-junior-level code to provide functionality that the user didn't ask for. Further, from an aesthetic point if view, you've limited the salability of your architecture, because your built in architectural premise is that each client gets a unique connection. If that premise breaks, you're going to be wanting a paddle.

So OK, beware of making a bound object Unreferenced, as the Unreferenced() method may be called multiple times. But with the ConnectionFactory, only the factory is bound. Each Connection instance returned by the factory's connect() method is a brand new instance, and only one client reference exists - from the client who did the connect(). In this context, a call to Unreferenced() seems pretty unambiguous - that client is done, period, and won't be called again. Does that make sense? Is there some other mechanism through which the count of remote references could increase from zero? My experience with RMI is pretty limited, so I could well be overlooking something.

I think the scenario, as described above, would do it. Unless I'm misunderstanding your question. Let me put it this way: your architecture, as it stands, will work(IMO, of course). but(again, IMO), it has more code then it needs to, more complexity then it needs to, and less scalability then it needs to. But, again, this is just my aesthetic, and it's certainly not valid then your own, not even to me .
All best,
M

[Jim Yingst]

[JY]: This is a problem - the lock() method defined in the API we must implement required an int, not Integer. We can't change that.
[MH]: Nor should we: However, that's not the map Vlad and I were talking about. Vlad isn't talking about Data, he's talking about his lock manager.
OK, I guess I need to understand this better. Does Data still have lock(int) and unlock(int, long) methods? Are they still used? Are they called by the LockManager, or do they call the LockManager's methods? Which object is actually enforcing the locks, and which is just forwarding calls? If a Client calls LockManager.lock(Integer), does that create a call to Data.lock(int)? Or is the record already locked once LockManager.lock(Integer) is called? If it's the latter, isn't this circumventing Data's API entirely (as far as locking is concerned at least)? I'm so confused...
----
Since I'm not sure I'm understanding how Vlad or Max's designs work here, I'll just talk about my own design while hoping for clarification. Swapping the order around a bit here...
[MH]: I don't think I understand what you're doing here. Can you elaborate a bit about what's in these maps?
Each Connection object has an internal HashMap of its own. The keys are Integers representing records that have been locked by that Connection; the values are Longs representing the cookie values that would be needed to unlock those records. To clarify: this data structure has nothing to do with enforcing locks (or the wait/notify that goes with them). That's handled by another HashMap inside Data, with entries for all locked records. The Connection's HashMap just helps the Connection know how to unlock any records it had previously locked, in case of disconnect. The Connection still needs to make a call to Data to actually perform the unlock().
[MH]: Your own code is limited by this. That is, if your Unreferenced code isn't synchronized on the hashmap, then it won't notify the map when you quitely remove an element. OTOH, if it is synchronized on the Map, it might never get called, because a client that's holding on the lock might die without ever calling notifyAll().
Sorry, I don't understand this. For what it's worth, I do access the HashMap from synchronized code, because RMI may allow multiple threads to be associated with a single Connection, and so I must protect mutable shared data. But why do we care about notify()/notifyAll() in here? Here's a simplified view of my close() method:

    public synchronized void close() {
        for (Iterator it = map.entrySet().iterator(); it.hasNext(); ) {
            Map.Entry entry = (Map.Entry) it.next();
            int recNo = ((Integer) entry.getKey()).intValue();
            long cookie = ((Long) entry.getValue()).longValue();
            db.unlock(recNo, cookie);
        }
        map.clear(); // in case close() is called again, no effect
    }

It's not performing any wait(), and so it doesn't need to receive any notify() or notifyAll() in order to function. You're not saying we need to receive a notify() in order to enter a synchronized block, are you? Or are you referring to the fact that other threads must receive a notify() if they were wait()ing for a record that the Connection unlocked? That's taken care of by db.unlock(). The db's lock() and unlock() implement a wait/notify, but that's using a separate monitor abstracted away inside the db instance. Not really relevant here, IMO.
----
[MH]: There's stuff going behind the scenes anyway: the gc, the dgc, the callbacks for Unreferenced, etc. IMO, and again, only MHO, the weakHashMap is a discreet bundle of isolation, written by Sun, that does everything we need here. I'm not sure I'm motivated to duplicate it's functionality.
OK, very valid point.
----
[MH]: RMI may decide to recycle the object to another client. Try this. Write 2 remote threads,
Is that two threads in the same remote JVM, or separate JVMs?
and give the object to one of them.
"Give" how? Are you talking about an object which has been bound, which any thread in a remote client can "get" with Naming.lookup()? That's what I do with my ConnectionFactory. Or are you talking about passing a remote object as a return value in a remote method invocation, as when ConnectionFactory.connect() returns a new Connection? Or are you talking about two threads in the same remote JVM passing a reference between them which refers to a single remote Connection_Stub instance on that remote JVM? Or some other means of "giving"?
Then, after the client releases the object, ref count hits zero. Now, give the object to the second client.
And here's where the method of "giving" is critical. If it's done via Naming.lookup() then yes the reference count went to zero and back, as I already discussed. If it's done using the return value of a remote method, then we have to look at where that method got the object. In the case of my ConnectionFactory's connect() method, it creates a new ConnectionImpl() every time it's called. The ConnectionImpl that the second thread talks to is not the same object that the first thread talked to. The new object has a reference count of 1, while the original object still has a reference count of 0. Or perhaps you mean something else?
When that second thread is finished(caput, dead, no longer with us, visiting Elvis), the ref count will again hit zero, and Unreferenced will again be called. The problem here is that you have to write fairly non-junior-level code to provide functionality that the user didn't ask for.
Well I still don't see how close() would be called again, but if it is, the map.clear() at the end ensures it won't attempt to unlock those records a second time. Doesn't seem too horribly non-junior-level.
Further, from an aesthetic point if view, you've limited the salability of your architecture, because your built in architectural premise is that each client gets a unique connection. If that premise breaks, you're going to be wanting a paddle.
Actually I've been assuming that muliple clients might eventually want to share a connection, and my Connection objects are indeed thread-safe. (Tested with all the same unit tests I ran on the original Data instance - I just had to put in an adapter to catch RemoteExceptions and convert the Connection back to a DB instance. No other special adaptations were necessary.) I normally talk about one client == one connection for simplicity, since that's all that's required for the assignment, and all that most people feel the need to talk about. But I can handle more if you want.
But regardless of how many clients share a connection - if that connection is lost, the server-side ConnectionImpl needs to unlock any associated records, or no one will ever be able to lock them again. (Where "ever" means "until someone restarts the server.") Unless of course we toss out disconnect handling entirely as too much work / not required. Though it's really not that difficult. This discussion may seem long and complex because we've got different competing versions of code in our minds - and can't see how the other person's vaugue description fits into what we were thinking. But looking at the complete code, it's not that hard to follow, IMO.
[ July 11, 2003: Message edited by: Jim Yingst ]

[Vlad Rabkin]

Hi Max,
Hi Jim,
First, it is correct what Max said, LockManager is my own class, so I can easily use there Integer, instead of int.

Second, it's possible that Unreferenced will be called several times, because the reference count can drop to zero, then back up again, then back to zero. Unreferenced, of course, is called whenever the reference count hits zero, not just the first time.

RMI may decide to recycle the object to another client. Try this. Write 2 remote threads, and give the object to one of them. Then, after the client releases the object, ref count hits zero. Now, give the object to the second client. When that second thread is finished(caput, dead, no longer with us, visiting Elvis), the ref count will again hit zero, and Unreferenced will again be called. The problem here is that you have to write fairly non-junior-level code to provide functionality that the user didn't ask for. Further, from an aesthetic point if view, you've limited the salability of your architecture, because your built in architectural premise is that each client gets a unique connection. If that premise breaks, you're going to be wanting a paddle

Unreferenced:
Now I am a bit confused. I know that RMI Server can reassign one remote object to another. Ok, so what? I have done following in My RemoteObject (DataRemote - Remote Impl of DB Interface):

 
  public long lock(int recNo){
...
             cookie = lockManager_.lock(recNo);
             locks_.put(new Integer(recNo), new Long(cookie));
...
}
  public void unlock(int recNo, long cookie){
...
              lockManager_.unlock(recNo, cookie);
             locks_.remove(new Integer(recNo));
...
}
   public void unreferenced(){
     java.util.Iterator it = locks_.keySet().iterator();
     while (it.hasNext()) {
        Integer recNo = (Integer) it.next();
        long cookie = ((Long) locks_.get(recNo)).longValue();
        lockManager_.unlock(recNo.intValue(), cookie);
     }
     locks_ = null;
     db_ = null;
     lockManager_ = null;
     logger_.info("**************************** Client is dead. Locks released. ");
    }  

, where locks_ is hashMap in RemoteObject which contains all currently opened locks by it client
So, if object has lost reference all locks will be released. If it will called, suddenly, many times, nobody cares, because locks_ is empty, so nothing will be done.
All complexity is about 8 rows of code.
Is this approach OK?
WeakHashMap:
I like this approach too(mainly because it will be called only once). In LockManager if unlock is called I use notifyAll (not notify), so if anybody unlocks , all the a locks which are waiting for already automatically removed Lock will go throght.
In order to implement the idea of Max, we have to have the reference on recoird ID in RemoteObject (That's why he suggested to keep it as Integer, not int...), but I beleive, we anyway must hold one additional HashMap or List in RemoteObject, which will contain all Integers IDs acquired by the client, because a client can acquire several locks
The only difference is that
in Unreferenced , I have to hold HashMap containing ID and Cookie and
I must write 8 rows of code in unreferenced method, 1 in lock() and 1 in unlock() additionally.
in WeakHashMap, I have told hold Collection containing only ID (I don't need cookies) and
I must write 1 row code in lock() mehods and 1 in unlock().
Is it correct?
Vlad

[Max Habibi]

/*

Originally posted by Jim Yingst:
[JY]: This is a problem - the lock() method defined in the API we must implement required an int, not Integer. We can't change that.
[MH]: Nor should we: However, that's not the map Vlad and I were talking about. Vlad isn't talking about Data, he's talking about his lock manager.
OK, I guess I need to understand this better. Does Data still have lock(int) and unlock(int, long) methods? Are they still used? Are they called by the LockManager, or do they call the LockManager's methods? Which object is actually enforcing the locks, and which is just forwarding calls? If a Client calls LockManager.lock(Integer), does that create a call to Data.lock(int)? Or is the record already locked once LockManager.lock(Integer) is called? If it's the latter, isn't this circumventing Data's API entirely (as far as locking is concerned at least)? I'm so confused...

In this design( which I do not advocate, even though I think it's acceptable) Data's lock/unlock pretty much go unused(am I correct here Vlad?). Basically, The LockManager contains metadata about the locks and the classes. It's an explicit redlight/greenlight controller that apart from the connections themselves.

Since I'm not sure I'm understanding how Vlad or Max's designs work here, I'll just talk about my own design while hoping for clarification. Swapping the order around a bit here...

Just want to be clear: this isn't my design, but it's a valid one. I'm just trying to help Vlad bullet proof it.

[MH]: I don't think I understand what you're doing here. Can you elaborate a bit about what's in these maps?
Each Connection object has an internal HashMap of its own. The keys are Integers representing records that have been locked by that Connection; the values are Longs representing the cookie values that would be needed to unlock those records. To clarify: this data structure has nothing to do with enforcing locks (or the wait/notify that goes with them). That's handled by another HashMap inside Data, with entries for all locked records. The Connection's HashMap just helps the Connection know how to unlock any records it had previously locked, in case of disconnect. The Connection still needs to make a call to Data to actually perform the unlock().

Ok, gotcha. This is almost exactly the approach I took with the new assignment.

[MH]: Your own code is limited by this. That is, if your Unreferenced code isn't synchronized on the hashmap, then it won't notify the map when you quitely remove an element. OTOH, if it is synchronized on the Map, it might never get called, because a client that's holding on the lock might die without ever calling notifyAll().
Sorry, I don't understand this. For what it's worth, I do access the HashMap from synchronized code, because RMI may allow multiple threads to be associated with a single Connection, and so I must protect mutable shared data. But why do we care about notify()/notifyAll() in here? Here's a simplified view of my close() method:

    public synchronized void close() {
        for (Iterator it = map.entrySet().iterator(); it.hasNext(); ) {
            Map.Entry entry = (Map.Entry) it.next();
            int recNo = ((Integer) entry.getKey()).intValue();
            long cookie = ((Long) entry.getValue()).longValue();
            db.unlock(recNo, cookie);
        }
        map.clear(); // in case close() is called again, no effect
    }

Ok, I can see where we're missing each other here. It's the ambiguously of 'hashmap'. We're getting caught up in which hashmap we're talking about. In the above, you're referring to the client specific hashmap, right? The one in your connector object? I was referring to your Unreferenced code, which deals with the Data's hashmap. Before I do too much further under my current assumptions, where does that unreferenced code live? That is, which class?

It's not performing any wait(), and so it doesn't need to receive any notify() or notifyAll() in order to function. You're not saying we need to receive a notify() in order to enter a synchronized block, are you? Or are you referring to the fact that other threads must receive a notify() if they were wait()ing for a record that the Connection unlocked? That's taken care of by db.unlock(). The db's lock() and unlock() implement a wait/notify, but that's using a separate monitor abstracted away inside the db instance. Not really relevant here, IMO.
----
[MH]: There's stuff going behind the scenes anyway: the gc, the dgc, the callbacks for Unreferenced, etc. IMO, and again, only MHO, the weakHashMap is a discreet bundle of isolation, written by Sun, that does everything we need here. I'm not sure I'm motivated to duplicate it's functionality.
OK, very valid point.

Jim, you must be a pretty good guy to work with. It's great when a engineer can accept valid ideas in a discussion, while still arguing about the ones they disagree with. Seriously, I think it's one of the hallmarks of senior people.

[MH]: RMI may decide to recycle the object to another client. Try this. Write 2 remote threads,
Is that two threads in the same remote JVM, or separate JVMs?

I don't think it matters, really. So long as it's not the server's JVM.

and give the object to one of them.
"Give" how? Are you talking about an object which has been bound, which any thread in a remote client can "get" with Naming.lookup()? That's what I do with my ConnectionFactory. Or are you talking about passing a remote object as a return value in a remote method invocation, as when ConnectionFactory.connect() returns a new Connection? Or are you talking about two threads in the same remote JVM passing a reference between them which refers to a single remote Connection_Stub instance on that remote JVM? Or some other means of "giving"?

Either using lookup, or simply returning the remote object.

Then, after the client releases the object, ref count hits zero. Now, give the object to the second client.
And here's where the method of "giving" is critical. If it's done via Naming.lookup() then yes the reference count went to zero and back, as I already discussed. If it's done using the return value of a remote method, then we have to look at where that method got the object. In the case of my ConnectionFactory's connect() method, it creates a new ConnectionImpl() every time it's called. The ConnectionImpl that the second thread talks to is not the same object that the first thread talked to. The new object has a reference count of 1, while the original object still has a reference count of 0. Or perhaps you mean something else?

This is I think I need some help understanding where your Unreferenced code lives. Is it in the connection, or Data?

When that second thread is finished(caput, dead, no longer with us, visiting Elvis), the ref count will again hit zero, and Unreferenced will again be called. The problem here is that you have to write fairly non-junior-level code to provide functionality that the user didn't ask for.
Well I still don't see how close() would be called again, but if it is, the map.clear() at the end ensures it won't attempt to unlock those records a second time. Doesn't seem too horribly non-junior-level.

Sorry to be confusing. Hopefully, when I get my head straight on where Unref lives, I can be more useful. I was working under the assumption that you were doing the same thing that some others are doing with Unreferenced, but it sounds like you have your own unique approach. Quelle surprise

Further, from an aesthetic point if view, you've limited the salability of your architecture, because your built in architectural premise is that each client gets a unique connection. If that premise breaks, you're going to be wanting a paddle.
Actually I've been assuming that muliple clients might eventually want to share a connection, and my Connection objects are indeed thread-safe. (Tested with all the same unit tests I ran on the original Data instance - I just had to put in an adapter to catch RemoteExceptions and convert the Connection back to a DB instance. No other special adaptations were necessary.) I normally talk about one client == one connection for simplicity, since that's all that's required for the assignment, and all that most people feel the need to talk about. But I can handle more if you want.

Ah, I see, because you synchronize access to the client's internal hashmap, right?

But regardless of how many clients share a connection - if that connection is lost, the server-side ConnectionImpl needs to unlock any associated records, or no one will ever be able to lock them again. (Where "ever" means "until someone restarts the server.") Unless of course we toss out disconnect handling entirely as too much work / not required. Though it's really not that difficult. This discussion may seem long and complex because we've got different competing versions of code in our minds - and can't see how the other person's vaugue description fits into what we were thinking. But looking at the complete code, it's not that hard to follow, IMO.
[ July 11, 2003: Message edited by: Jim Yingst ]

I think it's a pretty worthwhile discussion. I feel like I'm learning a lot, and challenging validating) some basic assumption. As the great Bob Marley said, it's all good .
All best,
M

[Jim Yingst]

In this design( which I do not advocate, even though I think it's acceptable) Data's lock/unlock pretty much go unused(am I correct here Vlad?). Basically, The LockManager contains metadata about the locks and the classes. It's an explicit redlight/greenlight controller that apart from the connections themselves.
OK. It's the circumvention of the lock/unlock that make me uncomfortable, but I'll agree it's probably legal, if not what Sun intended. Of course in other areas (like DuplicateKeyException) I'm perfectly happy to stop worrying about "intent" because I don't think the requirements writers knew what they wanted. Of they're being deliberately obtuse. So I guess I can't be too critical here.
Just want to be clear: this isn't my design, but it's a valid one. I'm just trying to help Vlad bullet proof it.
All right. I should note that I jumped onto this discussion because I shared Vlad's question about "where would you put the cookies". I thought that meant our designs were reaonably close to each other; on further examination it seems that they're differernt enough that I just created extra confusion by annexing my own questions onto the discussion; we've got too many different designs floating around here; it's hard to keep straight which assumtions we're using. So sorry for exacerbating the problem. But, since the damage has already been done, I might as well go on to clarify my own part in it.
Ok, gotcha. This is almost exactly the approach I took with the new assignment.
Ah, see, I knew there was a reason we made you a bartender.
Ok, I can see where we're missing each other here. It's the ambiguously of 'hashmap'. We're getting caught up in which hashmap we're talking about. In the above, you're referring to the client specific hashmap, right? The one in your connector object? I was referring to your Unreferenced code, which deals with the Data's hashmap. Before I do too much further under my current assumptions, where does that unreferenced code live? That is, which class?
For my own code, I'm making Connection imnplement Unreferenced. So when a given client connection is broken, the Connection associated with that client will receive a call to unreferenced() which allows it to clean up the locks associated with that client.
My previous comments did make mention of the possibility of the ConnectionFactory implementing Unreferenced, but that was just one of hte possibilities I was considering as an interpretation your own comments - it's not what I'm doing. Sorry, more extra confusion here...
Jim, you must be a pretty good guy to work with. It's great when a engineer can accept valid ideas in a discussion, while still arguing about the ones they disagree with. Seriously, I think it's one of the hallmarks of senior people.
Well to be honest I'm not always so accommodating. Maybe I'm just trying to get you to let your guard down. But thanks.
Ah, I see, because you synchronize access to the client's internal hashmap, right?
Yup. Had to do it because of the RMI Spec allowing multiple threads for a single client. But once it was there I realized that that meant that a single Connection could probably also support multiple clients if desired, and since I already had unit tests to simulate a bunch of different clients, it wasn't hard to adapt the test at that point. Multiple cients on a single connection isn't something I would have gone out of my way to allow, but as I seem to have implemented it accidentally, what the heck, it may be useful.
As the great Bob Marley said, it's all good .
Yup. As long as you don't decide to draw inspiration from a certain other Marley song.

[Max Habibi]

Originally posted by Jim Yingst:
Ah, see, I knew there was a reason we made you a bartender.

I though you needed a bald guy?

Ah, I see, because you synchronize access to the client's internal hashmap, right?
Yup. Had to do it because of the RMI Spec allowing multiple threads for a single client.

This still makes me a little uncomfortable: though I'm having a hard time putting my finger on it. What's the scenario where this manifests? As I understand it, RMI may use the same service thread to service several clients, but each client gets their own object, unless you explicitly decide to share. And if you did share, given your design constraints, then if GUI client #1 has records 4,5,6 locked, and GUI client #2 has records 7,8,9 locked, then how's client two to know supposed to know who has what? I'm missing something.

QUOTE]
As the great Bob Marley said, it's all good .
Yup. As long as you don't decide to draw inspiration from a certain other Marley song. [/QB]
lol, you made me spit up my tea!
All best,
M

[Vlad Rabkin]

Hallo Max,
Hallo Jim,
First to confirm:

In this design( which I do not advocate, even though I think it's acceptable) Data's lock/unlock pretty much go unused(am I correct here Vlad?). Basically, The LockManager contains metadata about the locks and the classes. It's an explicit redlight/greenlight controller that apart from the connections themselves

Exactly!
Jim, Max, sorry , but I am afraid I got lost in our discussion...
Could anyone confirm if I undestood your correctly:
1) Unreferenced Interface:
So, In addition to our lockmanagers HashMap,
we will have a Hashmap (In our RemoteObject, representing Data). This Hasmap will hold all active locks from the appropriate client. In
public void unreferenced()
we will go thougt all alements of this HashMap (active lock of the client) and force LockManager to unlock this locks.
Problems:
- it makes connection dependable on concrete client (something like Statefull Bean in EJB).
- unreferenced() can be called several times.
2) WeakHashMap - is much more flexible, but have to handle problem that as lock is removed from HasMap, all Waiters must be notified.
I don't understand here following: Our RemoteObject still must have own HashMap which have references to HashMap in LockManager: Integer IDs (Record Numbers), meaning that RemoteObject holds a "client state".
Is my assumtion correct?
Vlad

[Andrew Monkhouse]

Hi Vlad

1) Unreferenced Interface:
So, In addition to our lockmanagers HashMap, we will have a Hashmap (In our RemoteObject, representing Data). This Hasmap will hold all active locks from the appropriate client. In public void unreferenced() we will go thougt all alements of this HashMap (active lock of the client) and force LockManager to unlock this locks.

This may be getting more complex than it needs to be.
Jim did not have a LockManager. He was talking about having:

A map in the RemoteObject which was tracking which locks this particular client had locked, and it's cookies.

A map in the Data class which was tracking the locks and cookies for all connected clients.
This will work for the basic concept of clearing up after client dies. It will not handle deadlock situations (which has not been mentioned in this thread, but it is a possible reason why you might want a LockManager).
If you do decide to have a LockManager, then it would presumably need to know who the owner of the lock is as well as the lock number. If you also decide to store the cookie in the LockManager then you would not need to have the HashSet in the RemoteObject.

Problems:
- it makes connection dependable on concrete client (something like Statefull Bean in EJB).

Well you already have a concrete client - you are using a ConnectionFactory.

- unreferenced() can be called several times.

Yes, so you have to ensure that your code is safe.

2) WeakHashMap - is much more flexible, but have to handle problem that as lock is removed from HasMap, all Waiters must be notified.

I don't know if flexible is the term I would use. Perhaps "easier" for doing initial coding.
The problem, as you noted, is how to notify any waiting threads if a lock is removed from the WeakHashMap. Assuming this WeakHashMap is the only place you are storing which records are locked, then it is not too difficult. But if you are tracking locks in both your LockManager and in the Data class, then you will have problems with synchronization. I think Max and Jim have both come to the agreement that the lock in Data class is no longer being used in this case.
I can kind of think of another scenario where you have multiple instances of Data class (one per RemoteObject). In this case the Data class can create the Integer which gets stored in your WeakHashMap as suggested by Max. But in this case you probably want Data class to be just a facade to the single instance of the class doing your real work with reading and writing the database. Not sure if this is adding a layer of complexity that is unwarranted.

I don't understand here following: Our RemoteObject still must have own HashMap which have references to HashMap in LockManager: Integer IDs (Record Numbers), meaning that RemoteObject holds a "client state".

I think it is only when you are using Unreferenced that you would want to have a map of record numbers in the RemoteObject. If you are using the WeakHashMap solution, then I think that there is no value in the RemoteObject tracking locks (unless you want to be able to explicitly clean up outstanding locks if a client calls close()).
And as I mentioned before: if you track all three items (owner, record number, and cookie) in the LockManager, then you will not need to track them in the RemoteObject.
I don't like the idea of tracking locks in three different areas: there is too much chance that the annoying junior programmer will change one piece of code, possibly find the second one, but miss the third one. Then the program will have "really interesting" bugs.
Regards, Andrew

[Max Habibi]

Originally posted by Vlad Rabkin:
Hallo Max,
I don't understand here following: Our RemoteObject still must have own HashMap which have references to HashMap in LockManager: Integer IDs (Record Numbers), meaning that RemoteObject holds a "client state".
Is my assumtion correct?
Vlad

hi Vlad,
Sorry I got behind there. I think you've pretty much got the relevant issues, except for your question above. The answer is this: in your design, the remote client needs to keep track of which locks it has, and what cookies are associated with those locks: especially if you want to use Unreferenced to release them at a later time. That's the role of the second map.
HTH,
M

[Jim Yingst]

[AM]: I think Max and Jim have both come to the agreement that the lock in Data class is no longer being used in this case.
Umm, I think so. But I'm not using a LockManager at all (not something that's accessible outside the Data class anyway) and Max isn't a big advocate either, so I'm not sure it matters how much we agree on how it works. Other LockManager fans may hav their own ideas on how best to implement it.
[AM]: I don't like the idea of tracking locks in three different areas
I agree. I don't really like tracking them in two different areas, but the DB API seems designed to force me to, thanks to those silly cookies. Who am I to circument the will of the API writer? :roll:
[AM]: It will not handle deadlock situations
Interesting; somehow I'd managed to gloss over this thread when it was on its original topic, which was indeed deadlock prevention. My own preferred solution for deadlock prevention would be that if a client needs to lock more than one record, require them to acquire the locks in a particular order (e.g. by record number). If the client has already locked record 59 and then want record 14, they must unlock 59, then lock 14, then lock 59. This would be best handled by a facade attached to the client, IMO. Or I could incorporate in my Connection I suppose - though that would cause me to give up the ability to handle multiple clients with a single Connection. Oh well. Anyway though, I believe (a) the risk of deadlock is built into the DB interface; (b) it can be corrected with additional measures outside the DB class, but (c) such correction is unnecessary for this assignment because the clent never really needs multiple locks. Never even needs a single lock for more than a fraction of a second, really. So no worries as long as the client only uses the limited functionality provided by my GUI. If later they want to enhance it, fine, it's possible to safeguard against deadlocks without changing too much, and I've suggested a means. But it's not my responsibility to actually do that, IMO. Deadlocks aren't possible now; my job is done.

[Jim Yingst]

Hmmm... just after posting that last paragraph, I realized that the same arguments could be used to explain why there's no need to worry about client disconnects either. Almost. The main difference I can see between the two issues is that deadlock is totally impossible using the current client capabilities (locking only one record at a time), while client disconnect is not impossible, just improbable. That is, even if there is a disconnect, it's unlikely to occur in the brief interval between lock and unlock. (During which time I'm reading the record, checking that customer ID is still blank, and then updating with a new customer ID). But however brief this interval is, it's still possible for a disconnect to occur. And we don't want that to make the record completely unavailable until the server is restarted. So I can still rationalize worrying about client disconnects. But it's worth remembering that we can get away without it, as far as Sun's concerned.

[Jim Yingst]

[MH]: I though you needed a bald guy?
Yes, that too.
[MH]: Ah, I see, because you synchronize access to the client's internal hashmap, right?
[JY]: Yup. Had to do it because of the RMI Spec allowing multiple threads for a single client.
[MH]: This still makes me a little uncomfortable: though I'm having a hard time putting my finger on it. What's the scenario where this manifests? As I understand it, RMI may use the same service thread to service several clients, but each client gets their own object, unless you explicitly decide to share.
Previously we talked about 2 clients getting 1 thread, and you've described testing this, right? OK I admit this situation is sort of the reverse of that one - 1 client gets 2 threads. And I haven't tested it myself, but I believe the possibility is allowed by the RMI spec:

A method dispatched by the RMI runtime to a remote object implementation may or may not execute in a separate thread. The RMI runtime makes no guarantees with respect to mapping remote object invocations to threads. Since remote method invocation on the same remote object may execute concurrently, a remote object implementation needs to make sure its implementation is thread-safe.

Let's say client A has two threads c1 and c2, and both invoke the remote method foo() at approximately the same time. On the server side, each request can get handed off to a thread pool, and so it's not difficult to imagine that two different server threads s1 and s2 might field these two requests, is it? If a remote object were shared among multiple clients it would be getting simultaneous requests all the time, and so using a thread pool solution seems a good standard response. Why not just keep using this solution even if there's just one client?
Now, what if the client has only one thread, c1, and executes two sequential calls to foo()? Seems reasonable to think that the server might put these in the same thread, as we can guarantee that they two requests won't actually execute at the same time. (Since the client won't request the second foo() until it hears from the server that the first one has completed.) Seems reasonable, but I'm not sure the server knows or cares that the client is only using one thread. It just hands each incoming request off to some thread from the thread pool. Maybe it's the same thread as the last request; maybe not.
So, for a single-thread client, we can have two different server threads servicing remote calls. But they're not actually going to be execuint concurrently. Is this a problem? Does it require synchronizeation for thread safety? Maybe not, but I think so. Concurrent access isn't an issue, but there's still the issue with the Java memory model. (Yeah, that again.) Thread s2 may not be aware of the changes made by s1 if it's looking at a cached copy of the data. Unless we force a memory refresh with some sort of synchronization. To be fair, the thread pool itself probably has some synchronization in it (as part of the recycling process) which will prevent this from really being a problem. But as long as the RMI spec says "no guarantee" and "a remote object implementation needs to make sure its implementation is thread-safe", I'm not taking chances. Access to a shared HashMap will be synchronized.
All right, that was all about why I feel I must synchronize in the Connection, even if I don't decide to share the connection among clients. Now for the other half of the question...
And if you did share, given your design constraints, then if GUI client #1 has records 4,5,6 locked, and GUI client #2 has records 7,8,9 locked, then how's client two to know supposed to know who has what? I'm missing something.
Well, this is partly addressed in my previous deadlock response. The current GUI client can't lock more than one thing at a time anyway, and it's responsible for unlocking that record promptly. If future enhancements require clients to lock multiple records, and/or lock records for longer periods of time, then code to allow the client to keep track of its own records (and avoid deadlock by enforcing a locking order) can be put on the client. That is, on a facade that the client uses to interact with the Connection. Yes, the client gets a HashMap too. Don't want it to feel left out.
[MH]: lol, you made me spit up my tea!

[ July 14, 2003: Message edited by: Jim Yingst ]

[Vlad Rabkin]

Hi All,

And if you did share, given your design constraints, then if GUI client #1 has records 4,5,6 locked, and GUI client #2 has records 7,8,9 locked, then how's client two to know supposed to know who has what? I'm missing something.

Exactly that is the reason why I currently use additional HashMap in RemoteObject: to keep track on all active locks of the client.
I disagree with Jim that One client can acquire only one lock, and to acqure lock for anothoer record he mus release first one. At the moment it is right, but generally it is bad idea (let's we want in future allow the GUI to book several rooms at the moment?).

[MH]

the remote client needs to keep track of which locks it has, and what cookies are associated with those locks: especially if you want to use Unreferenced to release them at a later time. That's the role of the second map.

Ok, That is exactly what I have done, because I didn't want to hold client object in Lockmanager.
Max and Andrew said:
- referenced can be called many times
- Remote Object can be suddenly assigned to another client
and special care must ba taken.
My method currenly looks as follows:

public void unreferenced() {
java.util.Iterator it = locks_.keySet().iterator();
while (it.hasNext()) {
Integer recNo = (Integer) it.next();
long cookie = ((Long) locks_.get(recNo)).longValue();
LockManager.getInstance().unlock(recNo.intValue(), cookie);
}
locks_ = null;
db_ = null;
logger_.info("********* Client is dead. All its locks are released. ");
}  

, where locks_ is local HashMap on RemoteObject
which keeps track on all active locks of the client.
What kind of "special care" must be taken to avoid
"unlimited loops" and assignement of the remoteobject to another client by RMI server?

Thanx a lot!
Vlad

[Andrew Monkhouse]

Hi Vlad

What kind of "special care" must be taken to avoid
"unlimited loops" and assignement of the remoteobject to another client by RMI server?

As far as I can tell, there is the theoretical possibility that since you still have a reference to yourself when unreferenced() gets called, if you pass that reference to a (remote) object which tries to work with it, the reference count will be incremented and then decremented. So you would want to avoid this. It seems like you are not using a reference to "this" in your unreferenced code, so that is not a problem.
There is also the possibility that if your connection factory is maintaining a pool of connections, then the connection that just had unreferenced called could then farm that connection out to another client. (wow - that was a very badly written sentence: let me know if you want me to spell it out). Presumably to do this you would set some sort of flag in unreferenced to flag that this connection can be reused. If that happened, and your connection was reused, then sometime in the future it will become unreferenced again. Since you are destroying your map of locks, this would not affect you. I dont think this will affect you anyway, since I don't think you are pooling your connections - you are just creating a new connection on an as-needed basis. Aren't you? So it doesnt apply.
Regards, Andrew