NX: lock() and unlock()

[Nicholas Cheung]

After reading Max's book, I implement the locking in this way:
I allow the locking on records no matter exists/deleted or not:

public void lock(int recNo) throws RecordNotFoundException {
    Integer i = new Integer(recNo);
    boolean deleted = false;
    
    synchronized (recordLock) {
        try {
            while (isLocked(recNo)) {
              recordLock.wait();  
            }          
        } catch (InterruptedException ie) {
            throw new DataAccessException(ie.getMessage());
        }
        
recordLock.add(i);
recordLock.notifyAll();
    }
}

and the checking of the record status (valid or deleted) in the action method. (i.e. create(), update(), etc).
and the unlock() removed the recNo in the recordLocker:

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

However, when I double checked with instructions, it said:

Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.

What should I do? Should I need to check whether locking and unlocking an deleted/non-exists record??
If so, the locking code will be very complicated, while some deleted records cannot be unlocked.
Please advice.
Nick

[Andrew Monkhouse]

Hi Nick,
In your API documentation for the lock() method, do you have the instruction "Locks a record so that it can only be updated or deleted by this client."? If so, how are you ensuring that this is achieved?

Should I need to check whether locking and unlocking an deleted/non-exists record??

Yes, the instructions are quite clear that you have to do that.

If so, the locking code will be very complicated,

Not necessarily: you could add a method to verify if a record is valid, and just call that common method from your lock() and unlock() methods. This will keep your code clean and easy to read, and is a good practice.

while some deleted records cannot be unlocked.

This has been discussed multiple times in this forum. My preferred way of handling this would be to have the following:

    public void delete() {
        check if valid record
        check if locked
        delete record
        call private unlock
    }
    public void unlock() {
        check if valid record
        check if locked
        call private unlock
    }
    private void privateUnlock {
        unlock record
        wake up threads
    }

To use this, the client should only call "lock() ... delete()". The client should not call the unlock() method when doing a delete.
You can do some searches in this forum for other alternative ways of handling the delete and unlock problem.
Regards, Andrew
[ November 30, 2003: Message edited by: Andrew Monkhouse ]

[Nicholas Cheung]

Thanks Andrew.

In your API documentation for the lock() method, do you have the instruction "Locks a record so that it can only be updated or deleted by this client."? If so, how are you ensuring that this is achieved?

The client does not call those APIs directly. I provide a wrapper class, named DataServer to handle this.
When the delete(), update() and create() methods are invoked, the DataServer did the following:

public void delete(int recNo) throws RecordNotFoundException {
    data.lock(recNo);
    data.delete(redNo);
    data.unlock();
}

In data.delete(recNo), since I synchronized the RandomAccessFile, that means, no other I/O can perform, then it first check the validity of the record. If the record is valid, then it will be deleted, otherwise, RecordNotFoundException thrown. The data.unlock(recNo) removes the lock from the locker.
As from another post, Comments on lock/unlock please
If we do not synchronize RAF, the case that, the record is valid during the check, but is removed by other thread while the current thread is waiting for the lock. Even if the current thread checks again the validity of a record after it's wait() and before it's lock(), other thread can remove it in the mean time.

Not necessarily: you could add a method to verify if a record is valid, and just call that common method from your lock() and unlock() methods. This will keep your code clean and easy to read, and is a good practice.

I have had this function in my design, just don't know how to avoid the problem raise about.
If I sunchronized RAF in the beginning of the lock(), then, ALL I/O is blocked. But if not, something unexpected can happen between the check() and sleep(), and the check() and lock().

To use this, the client should only call "lock() ... delete()". The client should not call the unlock() method when doing a delete.

I do think this is a good idea. I will add this into my implementation.

By the way, should I provided create(), delete() functions to my wrapper class DataServer? At first, I do not have these functions becos the client will not perform such functions. But I feel it is very inconvenient to test the functions in Data without these (Since my test class actually need to implement those methods for testing purposes). Should I provided them in my submission, or I should remove them after the testing?
Thousand Thanks.
Nick.

[Nicholas Cheung]

In addition, I wanna ask, what should the unlock() checks?
1. Check whether the record is valid (deleted) in the data file? OR
2. Check whether the record is valid (deleted) in the dataLocker?
Thanks.
Nick.

[Jianhua Ren]

Nick,

Even if the current thread checks again the validity of a record after it's wait() and before it's lock(), other thread can remove it in the mean time.

How other thread can remove it after you got the lock of that record? If other thread's still working on that record, you cannot get the lock of that record. Once you get the lock of that record, it means no other threads are working on that record(lock of that record must has been released already). No need to synchronize the RAF.
Jianhua

[Nicholas Cheung]

Consider this:

public void lock(int recNo) {
  check(recNo);
  if (valid) {
      while (isLocked) {
        wait();
      }
      
      check again(recNo);
      // HERE: if not synchronized RAF, record can be deleted b4 get the lock
      get the lock;
  }
}

Do this case happen?

[Jianhua Ren]

Do you agree that every database operation has to follow the sequence lock/operation/unlock? If you agree, let's take a example step by step:
1. Collection of lockedRecords is empty
2. Thread T1 which wants to delete record 1 comes
3. T1's lock method gets called, lockedRecords contains one element representing record 1
4. At this moment, thread T2 which also wants to do something on record 1 comes
5. T2's lock method gets called, it checks the while condition("if Collection Contains 1"), it finds record 1 gets locked, then it goes into wait status
6. Thread T1 deletes the record 1 in the database, removes "1" from the lockedRecords(collection of lockedRecords is empty), and notifies other threads which wait for record 1 locks.
7. Thread T2 wakes up due to the notification from T1, checks the while condition("if Collection Contains 1"), condition is not satisfied, jumps out from the while loop
8. validates existence of the record 1, exception is thrown from validation method, since record1 does not exist.
9. collection is empty after all these steps.

How do you think?
Jianhua

[Andrew Monkhouse]

Hi Nick,

public void delete(int recNo) throws RecordNotFoundException {
    data.lock(recNo);
    data.delete(redNo);
    data.unlock();
}

In data.delete(recNo), since I synchronized the RandomAccessFile, that means, no other I/O can perform, then it first check the validity of the record. If the record is valid, then it will be deleted, otherwise, RecordNotFoundException thrown. The data.unlock(recNo) removes the lock from the locker.

Your sample code shows that DataServer.delete() calls unlock(), but then your text description says that Data.delete() calls unlock
If Data.delete() throws RecordNotFoundException, then the third line in your code above (the call to the data.unlock() method) will never be run.

If we do not synchronize RAF, the case that, the record is valid during the check, but is removed by other thread while the current thread is waiting for the lock. Even if the current thread checks again the validity of a record after it's wait() and before it's lock(), other thread can remove it in the mean time.

You do need to have access to the RAF synchronised, but I don't think that synchronizing on the RAF will prevent the problem you are talking about.
That is, in the following code:

    synchronized (raf) {
        checkRecordValid(); // throws BadException
        while (recordLocked(desiredRecord)) {
            raf.wait();
        }
        // do some work here
    }

While you are in the wait state, some other thread can still delete that particular record. The synchronization on the RAF does not prevent that. So you would still need to verify that the record is still valid after you have gained the lock.

By the way, should I provided create(), delete() functions to my wrapper class DataServer? At first, I do not have these functions becos the client will not perform such functions. But I feel it is very inconvenient to test the functions in Data without these (Since my test class actually need to implement those methods for testing purposes).

If you need these methods for verifying that the application is working correctly, then do add them.

Should I provided them in my submission, or I should remove them after the testing?

My personal feeling is that you should remove it afterwards. But I don't feel too strongly about it, so make up your own mind . My reasoning:

Having unneccessary code just gives the examiner something else that they might find fault with.

Having unused code just gives the examiner more to read - and they might get annoyed if they spend time working out what a particular method does only to discover that it is never used.

Extreme Programming suggests that all unused code should be removed from a project, because it distracts from the important code.

In addition, I wanna ask, what should the unlock() checks?
1. Check whether the record is valid (deleted) in the data file? OR
2. Check whether the record is valid (deleted) in the dataLocker?

Yes
If you are caching the record validity in the dataLocker, then by all means use it. But if not, then you will have to go back to the original data file.
Regards, Andrew