lock/unlock

[Miguel Roque]

Hello.
Please review my lock/unlock mechanism.
The Data class only has inside the lock and unlock methods the code that checks if the supplied record number is in the range of valid values (bigger than 0, less or equal than the record count and equal to -1 are OK). No real lock or unlock is done in this class.
In the server side, I have a class called RemoteDataImplementation that extends UnicastRemoteObject and implements my DataInterface. In this class I create a Data instance and a LockManager instance. Here, in the lock and unlock methods i call the lock and unlock from the Data class to check for the validity of the supplied record number and then I execute the lock and unlock from the LockManager that uses the client remote rererence. As the remote server has been created with a RMI factory, I have a different value for each client, so the client ID is allways a different number.
In the RemoteDataImplementation, the close method calls a method on the LockManager class that removes all the locks from a supplied client. This way I ensure that if a client dies and has the remote server uses the unreferenced interface to call the close method, no locks are keep in the Map.
The code for LockManager is as follows:

  
    private Map mapLocks = new HashMap();
    private final int DATABASE_LOCK = -1;
    private final Integer ALL_DATABASE_LOCKED = new Integer(DATABASE_LOCK);
    /**
     * Locks a given record by using the record number as key and connection
     * object passed to it as value. When the value of -1 is passed as
     * recNum, the entire database is locked.
     *
     * @param recNum - number of the record to be locked.
     * @param clientId - client that wants to perform the locking.
     */
     synchronized public void lock(int recNum, Object clientId) {
        Integer recordNumber = new Integer(recNum);
        if ( recNum == DATABASE_LOCK ) {
            while (!mapLocks.isEmpty()) {
                try {
                    System.out.println(clientId.toString()+" Waiting for all database unlock.");
                    wait();
                } catch (InterruptedException e) {
                }
            }
        } else {
            while (mapLocks.containsKey(recordNumber) || mapLocks.containsKey(ALL_DATABASE_LOCKED) ) {
                try {
                    System.out.println(clientId.toString()+" Waiting for record unlock.");
                    wait();
                } catch (InterruptedException e) {
                }
            }
        }
        mapLocks.put(recordNumber, clientId);
    }
    /**
     * Unocks the given record using the record number as key and connection
     * object passed to it as value. If the client doesn't have the lock
     * of this record, nothing is done.
     *
     * @param recNum - number of the record to be locked.
     * @param clientId - client that wants to perform the locking.
     */
    synchronized public void unlock(int recNum, Object clientID) {
        Integer recordNumber = new Integer(recNum);
        if (mapLocks.get(recordNumber).equals(clientID)) {
            mapLocks.remove(recordNumber);
            System.out.println(clientID.toString()+" Unlocked the record.");
            notifyAll();
        }
    }
    /**
     * Clears all pending locks for a client. This is used in the case
     * that a client crashes and leaves the connection open and
     * some records locked.
     *
     * @param clientId - client that wants to perform the locking.
     */
    synchronized public void clearClientLocks(Object clientId) {
        Iterator iter = mapLocks.entrySet().iterator();
        while (iter.hasNext()) {
            Map.Entry entry = (Map.Entry)iter.next();
            if (entry.getValue().equals(clientId)) {
                mapLocks.remove(entry.getKey());
            }
        }
    }

Please comment.
Thanks,
Miguel

[Peter den Haan]

Sweet. Virtually flawless, as far as I can see at a glance (mind you, with < -6 glasses, that's not very far at all).
mapLocks can be final, if you wish. The clearClientLocks method can be simplified and sped up ever-so-slightly by using iter.remove() instead of the call to mapLocks.remove().
- Peter

[John Smith]

Miguel,
Looks good, although I think that your catch (InterruptedException e) {} is a criminal offense. Silently ignoring the exceptions causes a lot of grief to the users. Beyond the grief, it will cause some serious damage in your implementation: if the thread running the lock() method is interrupted, it will actually perform the record lock!
Also, when the call lock(-1) is made, you don't really need to record it in the map, the class level boolean will do the same and the access code will look cleaner. But this is minor thing and is perhaps a matter of taste.
Eugene.
[ February 18, 2003: Message edited by: Eugene Kononov ]

[Eric Huang]

Eugene,
In your opion, how should we handle InterruptedException inside catch block?
Also, I want to know whether there is a possibility that, in lock method, a client try to lock a record that has already been locked by himself. If so, how should we handle it?

[Sai Prasad]

Originally posted by Eric Huang:
In your opion, how should we handle InterruptedException inside catch block?

Why not throw the exception and handle it in your Connection object.

Also, I want to know whether there is a possibility that, in lock method, a client try to lock a record that has already been locked by himself. If so, how should we handle it?
How about "public boolean isLocked(Object clientId)" in the LockManager class? Call this method before actually calling the lock() method. Or you could do the same checking in the lock() method itself. ie.. if the client tries to lock the same record twice, do nothing.

[Miguel Roque]

Hello Eugene and others.
After some 5 hour of sleep, here I am again.
Regarding the InterruptedException, you are right . I've never think of it that way.
I will put a return inside the catch block for the InterruptedException. Like this, no lock is performed if the thread is interrupted.
Thanks,
Miguel

[Peter den Haan]

Originally posted by Eugene Kononov:
Silently ignoring the exceptions [...] will cause some serious damage in your implementation: if the thread running the lock() method is interrupted, it will actually perform the record lock!

Eugene, can you elaborate on this? As far as I can see, the interrupt will simply be ignored, a valid lock will be granted. Whether this breaks something or not really depends on what interrupts are used for in the server.
Near the end of an earlier thread, I argued that how you handle interrupts is one of those design choices, and depends on what you determine interrupts can be used for in your classes. Other than Eugene, I regard ignoring interrupts as a valid design choice if perhaps not the best one.
Miguel, simply returning from the call is certainly not the way to handle interrupts. When a call to lock() returns, the client needs to be sure that he's actually got a lock on the given record. That's the contract of the lock() method. When an interrupt happens, the lock() method can either throw an exception (some type of DatabaseException, InterruptException or some flavour of RuntimeException; see the thread I linked to above), or ignore the interrupt and return after a lock has been acquired. Those are your only valid choices.
- Peter
[ February 19, 2003: Message edited by: Peter den Haan ]

[Miguel Roque]

Hello Peter.
Thanks for your repply and for the link to the other thread.
First I've added a notifyAll() in the clearClientLocks method so that when we remove any lock left from a dead client we will notify all the waiting threads.
Regarding the exception, the lock will catche the InterruptedException and will throw a DatabaseException to the Facade so that we can inform the client that a error has occurred and we could not complete the operation.
Please comment.
Miguel

[John Smith]

Peter den Haan wrote:
Eugene, can you elaborate on this? As far as I can see, the interrupt will simply be ignored, a valid lock will be granted.

Peter, you are absolutely right, -- no unintentional locking will occur, as I thought. I was just sloppy in reviewing the code, and somehow assumed that after the interrupt is caught, we would be out of the while() loop. This is not the case, of course.
I still don't like ignoring the exceptions, though. If the caller interrupts a thread, I think the best choice is to grant the interrupt.
Eugene.

[Peter den Haan]

Whoa! I hadn't picked up on the missing notifyAll(). Anyway, it looks great to me. With this implementation, you can at a hypothetical "later time" do things like breaking deadlocks by interrupting blocked lock() calls, etc. Eugene?
- Peter

[Peter den Haan]

Originally posted by Eugene Kononov:
I still don't like ignoring the exceptions, though. If the caller interrupts a thread [...]

I don't think RMI propagates a Thread.interrupt() over the wire, but I'm happy to be corrected on this. In other words, it wouldn't be the client interrupting the thread, but the server. I can imagine this could serve a useful purpose, e.g. in deadlock handling or server shutdown, although such pruposes are outside the scope of the FBN application.
- Peter

[John Smith]

Whoa! I hadn't picked up on the missing notifyAll(). Anyway, it looks great to me. With this implementation, you can at a hypothetical "later time" do things like breaking deadlocks by interrupting blocked lock() calls, etc. Eugene?

I think Miguel will outscore all of us (assuming that his MVC is right)
Eugene.

[Miguel Roque]

Hello Eugene.
The MVC is the other part of the story and I will came back later .
I belive that now I'm done with the review of the Lock/Unlock, it's time for my server design and this part is in other thread that i've created yesterday.
Only after this I will advance with my MVC implementation.
Thanks to all,
Miguel