Database Thread Safety, pls review

[Vlad Rabkin]

Hi,
This topic is based on the following discussion:
https://coderanch.com/t/183914/java-developer-SCJD/certification/design-should-lock-manager-synchronized
At least three possible solution for synchronization have been discussed there.
I want to provide two sample generic classes (at least I have tried to do them as much generic as possible). Both of them have nothing to do with lock/unlock methods, rather then synchronization issue to avoid data corruption.
I would like to ask you to review these classes, which represent different synchronization concepts. I would approciate any comments!
I also warn you not to reimplement my design, since it is not guaranteed to be good.
First sample class provides relative low level, fine-grained synchronitazion (ThreadsafeDatabase1). Synchronization is build in directly to the data access class.
The benefit of it is better performance since most read/write operation are synchronized
on record, but not on whole database. The disadvantage is that it is pretty dependable on data access methods and its design (I designed it for cashed database).

Second sample class provides coarse-grained synchronitazion(ThreadsafeDatabase3).
Synchronization is decoupled from data access class and is totally independant from it!
It is also relative simple: it allows concurrent read and exclusive write.
Disadvantage is the such calls like find will lock database for relative long time.
ThreadsafeDatabase1:

public class ThreadsafeDatabase1 {
  static private Map cash = new HashMap();
  static {
   cash.put(new Integer(1), new Object());
   cash.put(new Integer(2), new Object());
   cash.put(new Integer(3), new Object());
  }
  public Object read(int recNo) {
      Integer key = new Integer(recNo);
      // if a record is not in DB throw RecordNotFoundException
      checkRecord(key);
      // get the record from Collection
      Object record = getRecordFromCash(key);
      // We synchronize here to prevent any other thread from changing this record object:
      // our record reference is local (thread-safe), but the object it references is not.
      // Right?
    synchronized (record)  {
     // Do whatever I want with the record...
       return record;
      // Actually I return a clone here to return a copy of the object to the client
      // In that case I don't have to synchronize on record in find method.
      // return record.clone();
    }
  }
  public void update(int recNo, Object recordNew) {
      Integer key = new Integer(recNo);
      // validate request (not synchrnonized)
      validateRequest(recordNew);
      // get the record from Collection
      String[] recordOld = getRecordFromCash(key);
      synchronized (recordOld)  {
        // write the record in the database
        writeRecord(recNo, recordNew);
      }
      // update cash
      putRecordInCash(key, recordNew);
  }
  public Integer[] find (Object criteria) {
      // validate request (not synchrnonized)
      validateRequest(criteria);
      Integer[] keys = null;
      synchronized (cash)  {
        keys = new Integer[cash.size()];
        cash.keySet().toArray(keys);
      }
      // collection of ids of all records in the database
      Collection foundKeys = new TreeSet();
       for (int ids = 0; ids< keys.length; ids ++) {
          Object record = null;
         try {
          record  = read(keys[ids].intValue());
         } catch (NullPointerException recordNotFound) {
             continue;
         }
         // I beleive it is a dangerous point, because other thread can here
         // delete the record. If I try to synchronize on record which reference is null
         // it will cause NullPointerException!?
           synchronized (record) {
            // if record matches criteria:
            if (isMatching(record, criteria))
                 foundKeys.add(keys[ids]);
        }
       }
          // arrays of ids of all records in the database
          Integer[] foundIDsArray = new Integer[foundKeys.size()];
              foundKeys.toArray(foundIDsArray);
       return  foundIDsArray;
  }
  // checks if the record exists
  private void checkRecord (Integer key) {
    synchronized (cash)  {
      if (cash.containsKey(key))
           throw new NullPointerException("Record Not Found");
    }
  }
  // get record from collection
  private String[] getRecordFromCash (Integer key) {
    synchronized (cash)  {
      return (String[]) cash.get(key);
    }
  }
  // set record from collection
  private void putRecordInCash (Integer key, Object record) {
    synchronized (cash)  {
      cash.put(key, record);
    }
  }
  // write record in DB
  private void writeRecord (int recNo, Object record) {
    // NOT SYNCHRONIZED. It will be already called inside synchronized block
  }
  // throw Exception iin case of invalid request
  private void validateRequest (Object request) {
    // NOT SYNCHRONIZED. each thread has it own local copy of request.
  }
  // checks if the record in db matches the criteria
  private boolean isMatching(Object dbRecord, Object criteria){
    // NOT SYNCHRONIZED. It will be already called inside synchronized block
    return true;
  }
}  

ThreadsafeDatabase3:

public class ThreadsafeDatabase3 {
  private SinglethreadDatabase db = SinglethreadDatabase.getInstance();
 
  private ReadWriteLock rwManager = ReadWriteLock.getInstance();
  public Object read(int recNo) {
   Object record = null;
     try {
      rwManager.acquireReadLock();
        try {
          result = db.read(recNo);
        } finally {
            rwManager.releaseReadLock();
        }
     } catch (InterruptedException ie) {
          // throw IOInterruptedException();
     }
       return record;
  }
  public void update(int recNo, Object recordNew) {
     try {
      rwManager.acquireWriteLock();
        try {
          db.update (recNo, recordNew);
        } finally {
            rwManager.releaseWriteLock();
        }
     } catch (InterruptedException ie) {
          // throw IOInterruptedException();
     }
  }
  public Integer[] find (Object criteria) {
     Integer[] result = null;
     try {
      rwManager.acquireReadLock();
        try {
          result = db.find(criteria);
        } finally {
            rwManager.releaseReadLock();
        }
     } catch (InterruptedException ie) {
          // throw IOInterruptedException();
     }
       return result;
  }
   public long lock(int recNo){
     // no synchronitaion is needed, since lock method is synchronized on a hashmap
     // containing database record and acquires first its own lock to allow
     // writing in database
     db.lock(recNo);
   }
   public void unlock(int recNo){
    try {
      // read lock is required here, because
      // because unlock performes read in database to throw RecordNotFoundException
      // if a record not found.
      rwManager_.acquireReadLock();
          try {
            db.unlock(recNo);
          } finally {
              rwManager_.releaseReadLock();
          }
    } catch (InterruptedException ie) {
    }
   }
}
  

Thanx for your comments!
Vlad
P.S. Andrew, Max: I have tried to do my best to provide these classes in a generic manner. If you beleive I didn't achieved it,please let me know and I will immidiatelly remove the code.
[ August 21, 2003: Message edited by: Vlad Rabkin ]
[ August 21, 2003: Message edited by: Vlad Rabkin ]

[Jim Yingst]

I'm on my way out the door (will be vacationing about a week) but here are a few comments on ThreadsafeDatabase1 (since that's close to what I've been talking about):
It's "cache" not "cash".
I advise making separate classes for both the cache and the record. Most of your methods can be moved into one or the other, and then the sync can be entirely separate, which minimizes the chance of a junior programmer doing something stupid to create deadlock by improperly nesting locks. E.g. the main database code just does:

public void update(int recNo, Object recordNew) {
    Record rec = cache.get(recNo);
    rec.update(recordNew);
}

where the cache's get() is synced on the cache, and Record's update() is synced on the Record.
If the key to your cache is just an integer recNo, why use a HashMap? An ArrayList is faster and simpler, using recNo a the index of the List.
With caching, you need to decide if you want to just require that everything will be cached, or do you want to put in extra code to check for null and read from the db file if a record is not yet in the cache. This code can be hidden in the Cache class, which is nice. Note that I don't think that partial caching is very beneficial, as the biggest performance hog is the find() method, and evey time that's called it's going to try to read everything in the db. Which will usually force it to drop whatever else is in the cache, which means you get minimal benefit from the thing. So I don't bother with partial cahing - too much headache for little reward. But maybe you can find a better solution.
Defensive copying of any objects passed as parameters is a good idea, for all public methods that move data in or out of the cache. You never know what else the caller will do with that same object after you've cached a reference to it.

[Vlad Rabkin]

Hi Jim,
MANY THANX!
Have a good time in your vacation.
P.S. Thanx for warning me about small difference between cash and cache!

[Vlad Rabkin]

Hi,
Jim is in vacation, but I hope that somebody else can help to go further...

I advise making separate classes for both the cache and the record

Good idea, thanx. It will reduce confusing synvh blocks.

If the key to your cache is just an integer recNo, why use a HashMap? An ArrayList is faster and simpler, using recNo a the index of the List.

The difference is that Jim holds all deleted records in the List. So index number of an element refferes to the real record number. To simplify search I use HashMap, which allows me to hold only active records in cache.

So I don't bother with partial cahing

I don't want to it either.

Defensive copying of any objects passed as parameters is a good idea

Since I don't do it, I guess it is a tip
I commented a block in the first class, in the find method, which can bring a problem while multiple threads access it. Could anyone comment it?
Jim said in previous topic following:

Personally, I keep all records in memory, and I use an ArrayList (index is recNo) rather than a HashMap. To find(), I just loop through the ArrayList, synchonizing each individual access to the List; then after I've retrieved an individual record (and am no longer synced on the List) I sync on that record while I look at it. So there's no need for one long sync of the list; just lots of little ones.

Ok, I have partially changed the code as Jim said. Here is find method (TheadSafedatabase 1):

  public Integer[] find (Object criteria) {
      // validate request (not synchrnonized)
      validateRequest(criteria);

      int size = cache.size();
      Collection foundIDs= new TreeSet();
      for (int recNo = 0; recNo<size; recNo++) {
        
         // get method inside cache is synchronized on the whole Collection,
         // but I don't understand WHY?
         Record record = cache.get(recNo); 
        // What for should I synchronize cache.get(recNo):
        // Here before entering  synchronized (record) ...
        // Record Object can be changed by other thread !?
              
        synchronized (record) {
          if (!record.isDeleted() &&
              isMatching(record, criteria)) {
              foundIDs.add(new Integer(recNo));
          }
        }
      }
          // arrays of ids of all records in the database
          Integer[] foundIDsArray = new Integer[foundIDs.size()];
              foundIDs.toArray(foundIDsArray);
       return  foundIDsArray;
  }

It looks strange, but NOBODY ever commented my idea about read/write lock mechanism. Is it good ? Is is bad? Is it weard?
Max has one time commented it by saying that I should avoid multiple locks.
I have tested it many times. He was right I found a dead-lock if I would try to acquire readlock for invoking lock() method, but since it is not needed it is dead-lock safe.
I beleive it is very elegant and simple solution. Am I alone with this opinion?
Tx,
Vlad
[ August 22, 2003: Message edited by: Vlad Rabkin ]

[Vlad Rabkin]

Hi,
I provide here one more synchronization design for not cashed database.
The biggest merit of this design it is simple and easy understandable.
ThreadsafeDatabase2:

public class ThreadsafeDatabase2 {
 static private Object mutex = new Object();
  public Object read(int recNo) throws IOException{
    synchronized (mutex)  {
      // if a record is not in DB throw RecordNotFoundException
      checkRecord(recNo);
      // get the record from Collection
      Object record = readRecordDB(recNo);
     // Do whatever I want with the record...
       return record;
      // Actually I return a clone here to return a copy of the object to the client
    }
  }
  public void update(int recNo, Object recordNew) {
        // validate request (not synchrnonized)
        validateRequest(recordNew);
      synchronized (mutex)  {
        // if a record is not in DB throw RecordNotFoundException
        checkRecord(recNo);
        // write the record in the database
        writeRecordDB(recNo, recordNew);
      }
  }

  public Integer[] find (Object criteria) {
  // find is not synchronized: all member variables or db is made over read method,
  // which is synchronized. All other variables in the method are local ones, so
  // no need for synchronization.
      // validate request (not synchrnonized)
      validateRequest(criteria);
      // collection of ids of all records in the database
      Collection foundKeys = new TreeSet();
      int recNo = -1;
      boolean go = true;
      while (go) {
        recNo++;
          Object record = null;
         try {
          record  = read(recNo);
         } catch (NullPointerException recordNotFound) {
             continue;
         } catch (EOFException end) {
             break;
         } catch (IOException end) {
            // Nobody cares, it is just a sample
         }
            // if record matches criteria:
            if (isMatching(record, criteria))
                 foundKeys.add(new Integer(recNo));
      }
          // arrays of ids of all records in the database
          Integer[] foundIDsArray = new Integer[foundKeys.size()];
              foundKeys.toArray(foundIDsArray);
       return  foundIDsArray;
  }
  // checks if the record exists
  private void checkRecord (int recNo) {
   // IO if flag == deleted or record doesn't exist throw RecordNotFoundException
  }
  // read record from DB
  private Object readRecordDB (int recNo) {
    return new Object();
    // IO
  }
  // write record in DB
  private void writeRecordDB (int recNo, Object record) {
    // NOT SYNCHRONIZED. It will be already called inside synchronized block
  }
  // throw Exception iin case of invalid request
  private void validateRequest (Object request) {
    // NOT SYNCHRONIZED. each thread has it own local copy of request.
  }
  // checks if the record in db matches the criteria
  private boolean isMatching(Object dbRecord, Object criteria){
    // NOT SYNCHRONIZED.
    return true;
  }
 }

To my opinion at the cost of simplicity is performance: read and write operation block whole database every time.
Find is not synchronized, but I beleive it doesn't bring much, because it invokes read for each record, which is synchronized.
I could try to optimize it by prividing a synchronited Collection (e.g. Hashtable or Vector), which would have a pool of all record IDs. I could then synchronize on record ID, rather then on whole database (mutex in my sample), but I beleive it would require enhanced code for io access, because it can be not enough to synhcronize on record.
Can anybody answer: are there some isssue to be considered while synchronizing on a record instead of whole database?
Tx,
Vlad

[Andrew Monkhouse]

One more for the mix:

public class ThreadsafeDatabase4 {
    public synchronized Object read(int recNo) throws IOException {
        checkRecord(recNo);                    // throws RecordNotFoundException
        Object record = readRecordDB(recNo);    // get the record from collectn
        // Do whatever I want with the record...
        return record;
        // Actually I return a clone here to return a copy of the object to
        // the client
    }
    public synchronized void update(int recNo, Object recordNew) {
        validateRequest(recordNew);         // validate request (!synchrnonized)
        checkRecord(recNo);                 // throws RecordNotFoundException
        writeRecordDB(recNo, recordNew);    // write the record in the database
    }
    public Integer[] find(Object criteria) {
        // find is not synchronized: all member variables or db is made over
        // read method, which is synchronized. All other variables in the method
        // are local ones, so no need for synchronization.
        validateRequest(criteria);          // validate request (!synchrnonized)
        // collection of ids of all records in the database
        Collection foundKeys = new TreeSet();
        for(int recNo = 0; ; recNo++) {
            try {
                Object record = read(recNo);
                // if record matches criteria:
                if (isMatching(record, criteria)) {
                    foundKeys.add(new Integer(recNo));
                }
            } catch (NullPointerException recordNotFound) {
                continue;
            } catch (EOFException end) {
                break;
            } catch (IOException end) {
                // Nobody cares, it is just a sample
            }
        }
        // arrays of ids of all records in the database
        Integer[] foundIDsArray = new Integer[foundKeys.size()];
        foundKeys.toArray(foundIDsArray);
        return foundIDsArray;
    }
    // checks if the record exists
    private void checkRecord(int recNo) {
        // IO if flag == deleted or record ! exist throw RecordNotFoundException
    }
    // read record from DB
    private Object readRecordDB(int recNo) {
        return new Object();
        // IO
    }
    // write record in DB
    private void writeRecordDB(int recNo, Object record) {
        // NOT SYNCHRONIZED. It will be already called inside synchronized block
    }
    // throw Exception iin case of invalid request
    private void validateRequest(Object request) {
        // NOT SYNCHRONIZED. each thread has it own local copy of request.
    }
    // checks if the record in db matches the criteria
    private boolean isMatching(Object dbRecord, Object criteria) {
        // NOT SYNCHRONIZED.
        return true;
    }
}

Regards, Andrew

[Andrew Monkhouse]

Hi Vlad,
Thanks for all the effort in putting these samples together. Very well done.
Now I am going to start picking holes in them
In ThreadsafeDatabase1, just looking at the update method...

  public void update(int recNo, Object recordNew) {
      Integer key = new Integer(recNo);
      // validate request (not synchrnonized)
      validateRequest(recordNew);
      // get the record from Collection
      String[] recordOld = getRecordFromCash(key);
      synchronized (recordOld)  {
        // write the record in the database
        writeRecord(recNo, recordNew);
      }
      // update cash
      putRecordInCash(key, recordNew);
  }

As I see it, two separate threads could both try and update the same record at the same time. They could both get to your synchronized block at the same time. Thread A does the write to the database, exits the synchronized block, then gets swapped out. Thread B does it's write to the database and updates the cache and exits. Thread A then gets swapped back in, and updates the cache. Now we have the cache not matching what is in the database.
I think ThreadsafeDatabase1 has a few too many of these types of holes. Plus for any given user function, there may be several calls to internal functions which are synchronized. This will require a lot of overhead as locks are created and destroyed.
Now for the next one...

[Andrew Monkhouse]

Looking at ThreadsafeDatabase2. This has the advantage that some operations (for example validating criteria to be found or data to be updated) can proceed while any operations on the file itself (read or write) will be kept safe because of the mutex.
This is probably my favourite example.
Regards, Andrew

[Andrew Monkhouse]

Now to look at ThreadsafeDatabase3.
I assume that in this case that no thread can aquire a write lock as long as there are any outstanding read locks. I think that this could potentially cause starvation in the threads trying to write. You might have to write a queueing mechanism in ReadWriteLock to ensure that starvation does not occur.
(Note: that is more a theoretical concern than anything else in this theoretical discussion. For the SCJD assignment, you might get away with just a note in your code about the potential for starvation, and leave it to a future enhancement to reduce this potential.)
Now if this were modified slightly so that the read or write locks were granted on a particular record number, then this might be better. Of course this would probably mean a change to how the find method works, as it is going to have to get and release locks for every record.
I don't really understand why lock() does not need synchronization, but unlock() does.
Regards, Andrew
[ August 22, 2003: Message edited by: Andrew Monkhouse ]
[ August 23, 2003: Message edited by: Andrew Monkhouse ]

[Andrew Monkhouse]

Finally ThreadsafeDatabase4.
This is just a modification of ThreadsafeDatabase2. The mutex has been removed in favour of synchronizing the method.
This makes the code a tinier bit simpler (you don't have to explain to the junior programmer what a mutex is ) but the disadvantage is that the lock exists even while some operations that dont require the lock are being performed (such as validating the data).
Regards, Andrew

[Andrew Monkhouse]

Hi Vlad,

I commented a block in the first class, in the find method, which can bring a problem while multiple threads access it. Could anyone comment it?

No, this will not cause an exception. If the value in the set is changed or deleted, your reference will still point to the old value. So your synchronization will still work (even though you would then be synchronizing on an object that you alone have).
Regards, Andrew

[Andrew Monkhouse]

Everyone else,
I strongly recommend you look at Vlad's samples. See if you can spot any issues. See if you agree with my comments. See if you can improve on them in any way. See if you can find another way of doing this. This is a great opertunity to learn from the hard work Vlad has put into this, and this is applicable to the assignments.
And please - if you have any questions - ask them!
Regards, Andrew
[ August 23, 2003: Message edited by: Andrew Monkhouse ]

[Vlad Rabkin]

Hi Andrew,
Tx for your comments and suggestions.
Here are some my thoughts and answers to your points mentions by you.
ThreadSafeDatabase1:

[Andrew] I think ThreadsafeDatabase1 has a few too many of these types of holes.
[Jim] ... after I've retrieved an individual record (and am no longer synced on the List) I sync on that record while I look at it.

Yeas, I agree with Andrew, because nobody knows what can occur between these two synchronized blocks, but I am not able to discuss it(neither advocate, nor critisize), since I don't know exactly Jims implementation details.

ThreadSafeDatabase2/4:
Ok,
I think Andrew is right: Synchronizing on this is even simplier (but it will work only if Data class is a singletone or mutition).
Andrew, does it bring a lot not synchronizing find method?
Find is not synchronized (so such operation like isMatching() can run concurrently, but find invokes read for each record, which is synchronized.
Does it make sense?
ThreadSafeDatabase3:
[Andrew]

I assume that in this case that no thread can aquire a write lock as long as there are any outstanding read locks. I think that this could potentially cause starvation in the threads trying to write.

Yes and No. I wanted to hide details of RWLock Class, since there are many possible implementations of it (which is one more merit of this approach).
My implementation uses Preferenced Write: That means that if no active locks exists, a write lock will be preffered over read lock. Because of that the situation described by Andrew is ALMOST impossible:

  private boolean allowReader() {
    return activeWriters_ == 0 && [B]waitingWriters_ == 0[/B];
  }
  private boolean allowWriter() {
    return activeReaders_ == 0 && activeWriters_ == 0;
  }
  public synchronized void acquireWriteLock()
   throws InterruptedException {
    ++waitingWriters_;
     while (!allowWriter()) {
     try {
      wait();
     } catch(InterruptedException ie) {
  --waitingWriters_; // roll back
      throw ie;
          }
     }
    --waitingWriters_;
    ++activeWriters_;
  }

[Andrew]

Now if this were modified slightly so that the read or write locks were granted on a particular record number...

I think it would one of the best synchronitaion approach, but I think it is a bit hard to implement it. Synchronizing on a record will allow concurrent write for different records. It means
1) we would better then don't use Data singleton and RAF: I use FileChannel and mostly local variables in my Data write methods, so it shouldn't be a problem, but for those who don't do it can be a problem.
2) We would have to have 3 types of locks:
- record read lock
- record write lock
- database write lock (for add/create method)
For FBN assignement it is Ok, since it requers explicitly anyway a database lock, but for other assignement it is overhead to maintain 3 different types of read/write/database locks.
From other side, it would be probably the best synchronization mechanism from the mentioned above...
[Andrew]

I don't really understand why lock() does not need synchronization, but unlock() does.

I needed to 3 days to find dead-lock reason and get this idea, otherwise acquiring read lock for lock() would cause dead-lock.
All assignement have different requirements. Mines have requirement that lock()/unlock() must throw RecordNotFoundException first if the a record is not in DB, or its flag is set to be deleted. So I have to read from cash/db to check it. That is why I need to acquire read lock first. Why don't I need than to do it for lock() invokation?
Because I first acquire a lock (not a read or write lock, but a user lock for lock/unlock methods!) and then I read from cash/db.
I don't need to acquire read lock, because acquiring a user lock guarantees that nobody can change a record)!!! That is probably the most confusing point in ThreadSafeDatabase3. I provide here a lock method from Data to make it clear:

     public long lock(int recNo)
       throws RecordNotFoundException, ... {
       synchronized (cashedDB_) {
         long cookie = 0;
              Integer recordNumber = new Integer(recNo);
             while (locks_.containsKey(recordNumber)) {
               cashedDB_.wait();
             }
        
              // it is save NOW to read from db: nobody can delete or update the record, 
              // the only theoretocal danger is probably create, because it doesn't need
              // a user lock to write in db.
              checkRecord(recNo);
                cookie = ...
                // put the lock in the pool of all locks
                locks_.put(recordNumber, new Long(cookie));
                  return cookie;
       }
     }

Unlock() need read lock, because it is not theotically clear if the caller really ownes the lock (in Case of SecurityException).
Regards,
Vlad
[ August 25, 2003: Message edited by: Vlad Rabkin ]

[ August 25, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Andrew and Vlad
Andrew

I assume that in this case that no thread can aquire a write lock as long as there are any outstanding read locks. I think that this could potentially cause starvation in the threads trying to write. You might have to write a queueing mechanism in ReadWriteLock to ensure that starvation does not occur.

I have a similar approach with a separate class dedicated to synchronizing (I use two instances of it, one for global access synchronizing and one in my Cache class). I solved the problem you mention by giving a higher priority in granting write locks than read ones. It means that if a write lock claim occurs while reads are in progress the caller waits till all reads are done, but new read lock claims wait till the write lock is granted and released.
There is another advantage in favor of that solution : if a write operation is pending, it seems better to read after the write than before it.
But I now read that it's what Vlad does too...
Best,
Phil.

[Vlad Rabkin]

Hi Phil,
I am glad to hear you again!
Phil,
One question:
To avoid dead-lock I do not acquire read lock for lock() (see my last message). It is actually safe because no update/delete is possible after user lock is acquired. The only problem is create method, since it doesn't need to acquire a user lock it can be a problem:
1) Thread A ivokes lock for a record #33. The record doesn't exist at the moment. Method lock() first acquires lock for #33. It breaks through that block:

              while (locks_.containsKey(recordNumber)) {
               cashedDB_.wait();
             }
 

2) Thread B invokes acquires write lock (not a user lock) and calles
create() method. No deleted records are found and it creates record #33.
3) Thread A reads from DB to find record #33 either to put the record in locks pool (hashmap) or to throw RecordNotFoundException.
We have the situation where concurrent write (Thread B: create; Thread A: read to find a record) occur.
4) Thread A either will find the record #33 (if Thread B is fast enought to put it in cash) or it will read garbled data (since booth threads run concurrently), which will lead to unpredictable results. These both results are incorrect, because Thread A is supposed to throw RecordNotFoundException.
Is it OK? If not, how do you manage it? (The only idea what I currently have is to synchronize create method inside data on cached database object, since both lock/unlock methods are also synchronized on that object, but I guess I have a bit different design, since you have separate class for LockManager, where lock/unlock are synchronized on "this").
Tx,
Vlad
[ August 25, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Vlad,
Glad to read you again too !
In my (new) implementation, I think the bad scenario you describes from 1) to 4) couldn't happen.
Here is my (new) lockRecord method :

    public long lockRecord(Object connectionObj, long recNo) throws
        RecordNotFoundException {
        long cookie = lockManager.lockRecord(connectionObj, this, recNo);
        try {
            mainSynch.beginRead();
            if (!isRecordNumberValid(recNo)) {
                lockManager.unlock(connectionObj, this, recNo, cookie);
                throw new RecordNotFoundException("Record number " + recNo +
                                                  " is invalid");
            }
            if (isDeleted(recNo)) {
                lockManager.unlock(connectionObj, this, recNo, cookie);
                throw new RecordNotFoundException("Record number " + recNo +
                                                  " is deleted");
            }
            else {
                return cookie;
            }
        }
        finally {
            mainSynch.endRead();
        }
    }

And here is the isRecordNumberValid method :

    private boolean isRecordNumberValid(long recNo) {
        return (recNo >= 1) && (recNo <= (recCount + deletedRecords.size()));
    }

... and the isDeleted method :

    protected boolean isDeleted(long recNo) {
        return deletedRecords.contains(new Long(recNo));
    }

As my LockManager.lockRecord method does not care about the existence of recNo in the table,
the lock will be granted immediately. As you can see in the code, if a record is created (or deleted) in the meantime, it will be taken into account.
Thanks a lot for having pointed the issue out because my lockRecord() method was buggy before I read you !
Best,
Phil.
[ August 25, 2003: Message edited by: Philippe Maquet ]

[Vlad Rabkin]

Hi Phil,
I use very simular concept. I am glad I can be usefull for you too, since you have helped me all the time!
As I said having it vice versa inside lockRecord method:
1.) mainSync.beginRead
2.) lockManager.lockRecord
would 100% lead to dead-lock (I have test it and if you want I can provide scenaria how it cann occur).
You have just suggested probably a good solution. I will try to analyze it and test it now. I really hope it would not cause a deadlock...
The only disadvantage I see at the moment: is mixing mainSync with Data. It was my argument to use it in a separate ThreadsafeData class, since it allows 100% to decouple synchronisation issues and database access logic (together with lock/unlock functionality).
I will write a bit later today.
Tx,
Vlad
[ August 25, 2003: Message edited by: Vlad Rabkin ]

[Vlad Rabkin]

Hi Phil,
I am still testing your idea. I get a dead-lock while testing multiple create() and delete() threads. I am investigating now the cause : I want to make sure I have not done any other error.
Vlad.

[Andrew Monkhouse]

Hi Vlad,

ThreadSafeDatabase2/4:
Ok,
I think Andrew is right: Synchronizing on this is even simplier (but it will work only if Data class is a singletone or mutition).
Andrew, does it bring a lot not synchronizing find method?
Find is not synchronized (so such operation like isMatching() can run concurrently, but find invokes read for each record, which is synchronized.

The simplicity offered does come at a price: the synchronizations are heavy operations, and we are jumping in and out of the synchronized blocks a large number of times for the find. But the advantage is that other operations will not be blocked until after the find is complete. I think this is something that in real life we would have to go back to the client and ask them whether they want the find to complete in the shortest available time at the expense of other operations, or whether they want other operations to potentially complete before the find completes.
For this assignment, in most cases performance is not an issue, but simple, easy to read code is desirable.
Regards, Andrew

[Andrew Monkhouse]

Hi Philippe,
Glad you joined in.
I am curious: why do you have three paramaters to your LockManager.lockRecord() method?
I assume that your create() method does not try to reclaim space left by deleted records? If it did - then do you have a potential for a user to request a lock on a deleted record (thereby creating the entry in the lock manager) after which a new record is created in that record number which can never be locked?
Regards, Andrew

[Vlad Rabkin]

Hi Phil,
Ok, I found a reason for the dead-lock. It was my fault. My Data.lock method was synhcronized (on cashed database). Acquiring a read lock inside the synchronitazion lock (nested lock) lead to the dead-lock.
Having it like you did will NOT cause problems.
It will lead to the dead-lock:

 
public .. lock (..) {
synchrnozited (cache) {
lock()
mainRW.beforeRead();
 try {
 checkRecord(); // throw RecordNotFoundException if doesn't exist
 } catch (RecordNotFound rnf) {
      unlock();
      throw rnf;  
  }
mainRW.afterRead();
 }
} 

It will NOT lead to the dead-lock:

 
public .. lock (..) {
synchrnozited (cache) {
lock()
}
mainRW.beforeRead();
 try {
 checkRecord(); // throw RecordNotFoundException if doesn't exist
 } catch (RecordNotFound rnf) {
    synchrnozited (cache) {
     unlock();
      throw rnf;  
    }
 }
mainRW.afterRead(); 
}

Best,
Vlad

[Philippe Maquet]

Hi Andrew,

I am curious: why do you have three paramaters to your LockManager.lockRecord() method?

My design is :
DBAccess == Data \
DBAccess == Data --> Table --> LockManager
DBAccess == Data /

Data receives a connection Object in its contructor and passes it to Table.lockRecord from its own lockRecord method.
Remember that it's the solution I found out to get rid off the issue of identifying lock owners by Thread.currentThread().

I assume that your create() method does not try to reclaim space left by deleted records? If it did - then do you have a potential for a user to request a lock on a deleted record (thereby creating the entry in the lock manager) after which a new record is created in that record number which can never be locked?

No, I do reclaim space left by deleted records. But as create (as delete and update) uses a "beginWrite() .. finally endWrite()" construct, the problem you point out cannot happen.
But the issue pointed by Vlad shows that lockRecord() cannot be called within a beginRead() .. endRead() :
(BR/ER here mean beginRead()/endRead() and BW means beginWrite())

A : wants to delete record r
A : lockRecord() - locks granted immediately
B : wants to lock record r
B : BR + LockManager.lockRecord + wait
A : BW in deleteRecord() will hang

My corrected lockRecord method is now OK (see above).
Best,
Phil.

[Philippe Maquet]

Hi Vlad,
That's what I thought. Thanks again for that "debug brainstorm".
Regards,
Phil

[Vlad Rabkin]

Hi Andrew,

The simplicity offered does come at a price: the synchronizations are heavy operations, and we are jumping in and out of the synchronized blocks a large number of times for the find. But the advantage is that other operations will not be blocked until after the find is complete.

Ok.
From simplicity point I beleive your design is the best one.
Now I have to decide then between Theadsafedatabase2/4 and Theadsafedatabase3, but I guess nobody could help me to take this desicion.
Andrew, thanx a lot.
Vlad
P.S. Phil, I also wonder why do you have so many arguments in a lock method:
(connectObj, this)?

[Philippe Maquet]

Hi Vlad,

P.S. Phil, I also wonder why do you have so many arguments in a lock method:
(connectObj, this)?

For connectionObj, I replied above. Now "this" is the Table. My LockManager class is a singleton, my database is "multi-table-designed" and in such a system a given record is uniquely identified by a table/record number pair.
Best,
Phil.

[Vlad Rabkin]

Hi Phil,
Ok. Well, you are very creative person!
Best,
Vlad

[Vlad Rabkin]

Hi Phil,
I am just thinking again which design to use.
The one we use (RWLock) is very elegant and it allows concurrent read.
I wanted to use it because of main reason: it should allow to decouple Data class and its synchronization class, so that IO deveoper should care about synchronization at all. It was the biggest advantage to other design.
As we just have found the bug in lock method, it doesn't allow us anymore to decouple synchronitazion and Data Access class.
The biggest advantage of design from Andrew is simplicity, disadvantage: it doesn't allow concurrent read. Andrew has very painfull (for me) argument: simplicity is an issue, not a performance.
I just wanted to know all your thought and how you are going top justify readwritelock synchronization strategy against others. How are you going to justify that it is good idea to have in Data class directly?
I know, we can say it is a good idea to have all these thing hidden in Data class. From the other hand, it makes extremely hard to test Data access (because you don't know where the problem is: in io or in synchronization) and it contradics OO concept (put different thing in different classes...).
Tx,
Vlad

[Philippe Maquet]

Hi Vlad,

The one we use (RWLock) is very elegant and it allows concurrent read.

I am so happy to agree with you !

I wanted to use it because of main reason: it should allow to decouple Data class and its synchronization class, so that IO deveoper should care about synchronization at all. It was the biggest advantage to other design.

I don't understand what you mean by "to decouple Data class and its synchronization class". We are simply replacing the built-in Java synchronizer which is single-read/single-write (any form of the "synchronize" keyword), by a multiple-reads-single-write synchronizer of our own. Nothing more IMO.

As we just have found the bug in lock method, it doesn't allow us anymore to decouple synchronitazion and Data Access class.

The bug had nothing to do with our way of synchronizing : if you replace in our former buggy lockRecord() method our beginRead()..endRead() block by a synchronized(someMonitor) block, you keep the bug !

The biggest advantage of design from Andrew is simplicity, disadvantage: it doesn't allow concurrent read. Andrew has very painfull (for me) argument: simplicity is an issue, not a performance.
I just wanted to know all your thought and how you are going top justify readwritelock synchronization strategy against others. How are you going to justify that it is good idea to have in Data class directly?

Well, I disagree here with Andrew (it's so rare ! ) if you except the fact that we had to design and code that one more class. JavaDoc comments included, my MultiReadSingleWriteSynchronizer class in 200 lines long : not a big deal ! And once you have it at hand, writing :

try {
    synch.beginRead();
    ...
    ...
}
finally {
    synch.endRead();
}

is not that much more complex than :

synchronized(this) {
   ...
   ...
}

Now there is a huge difference in terms of performance. Just an example :
I implemented a Cache (probably too complex IMO : optional maximum size, cleanup process which takes the number of hits per record and their last access time into account to choose the right candidates to be cleaned up - but that's another topic) and that Cache uses a backing HashMap. Well, thanks to its private MultiReadSingleWriteSynchronizer instance, my Cache allows concurrent reads, which is quite a minimum requirement for a read cache, right ?
Would it make sense to build a caching system - worst a rather complex one - to boost performance, if it could serve only one record at a time ?!
I have a question for you : how did you handle the InterruptedException in your RWLock class ?
I tried to understand first what an InterruptedException is. It seems that it is only thrown when Thread.interrupt() is called on a thread which is in some wait state (wait(), sleep(), join()). The interrupt() method sets an interrupt status (a flag) which can be tested by the isInterrupted() method. Now my tests have shown that when an InterruptedException is thrown the interrupt status is cleared (the isInterrupted() method returns false). So I decided to reset it myself by calling Thread.currentThread().interrupt() from within the catch block (without throwing any further exception) :

                try {
                    wait();
                }
                catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                }

The idea behind it is that if the behaviour caller thread is based on its interrupt status, like here :

    public void run() {
        while (!isInterrupted()) {
            ...
            ...
            ...
        {
    }

its behaviour will not be influenced by the use of my MultiReadSingleWriteSynchronizer class. I documented it in JavaDoc this way :

 * <p><b>Thread interruption transparency</b><br>
 * If the calling thread is interrupted while waiting for the lock it claims
 * for, the called method returns immediately with an unaffected
 * <i>interrupted status</i>. It means that any thread whose behaviour
 * relies on <code>Thread.isInterrupted()</code> may safely use a
 * <code>MultiReadSingleWriteSynchronizer</code> instance.
 * </p>

I personally don't use Thread.interrupt() in my own code, but as MultiReadSingleWriteSynchronizer is a general-purpose class, that "thread interruption transparency" seemed good practice to me, but I have not enough Java experience to be sure.
So any comment will be welcome !
Best,
Phil.

[Vlad Rabkin]

Hi Phil,
You can't imagine how wonderful it is to see at least somebody advocating the same design as I have !

public String[] read (int recNo) {
 rwmanager.beginRead();
   return data.read(recNo);
 rwManager.endRead();
}
 

In this case Data class developer doesn't care at all about threads and synchronization. He can't just do anything wrong!
If there is a bug in IO you test Data. If there is a bug in synchronization - you test ThreadSafeData.
As we have to build in rwLock management inside lock() method of Data, ThreadSafeData losses its sense. So, we put everything into Data.
You are right: using synchronized(...) would be also in Data, but that is what exactly the advantage of RWLock and ThreadSafeData was supposed to be ...

 
  public synchronized void acquireReadLock()
   throws InterruptedException {
    ++waitingReaders_;
    while (!allowReader()) {
     try {
      wait();
     } catch(InterruptedException ie) {
  --waitingReaders_; // roll back
      throw ie;
          }
     }
    --waitingReaders_;
    ++activeReaders_;
  }

2. is Data class.
I catch this exception. I create new IOInterruptedException (Max have done almost the same thing in his book). I wrap this exception in my RuntimeException and throw myRuntimeException:

        }catch(InterruptedException ie){              
            InterruptedIOException iie =
                new InterruptedIOException("Threading Error");
           throw new MyUncheckedException(iie);
        }

3. DataFacade

public RecordModel read (int recNo) throws RecordNotFoundException,
                                           IOException {

        }catch(MyUncheckedException my){              
           throw new IOException(my.getMessage());
               or
           throw (IOException) my.getCause();
        }
}

I beleive it is extremelly elegant solution because:
My Facade Interface (Business tier as you call it) has all standard exceptions plus IOException. Implementation of this DBFacade interface catches all MyUncheched exception takes an original exception (IOException) and throw it. By parsing InterruptedException into IOInterruptedException does not require to add one more exception in signature of my methods in Facade, since IOInterruptedException is a child of IOException.
This is one of the very few points in the assignment, that I like myself
I beileive invoking interrupt method yourself it a bad idea (at least all the books I have read say it). So, I don't do it. I do not check if a thread is dead or so: it is confusing and is not needed, since I don't invoke interrupt on the threads. I guess this exception can only happen if a client server thread dies (I don't know if its possible). If it is happens I cleanly rethrow this exception and deliver it to the client as it must be (as I described it above). For the client should handle this problem exactly as an IOException, so parsing it to IOInterrupted exception is a wonderful solution.
Phil, please, if you don't understand my point ask me again (since my art of asking and answering questions is not the best )
Best,
Vlad
[ August 26, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Vlad,

You can't imagine how wonderful it is to see at least somebody advocating the same design as I have !

Yes I can imagine as I feel the same thing. You're right, let us congratulate us while we are just the two of us talking to each other : Max, Andrew or Mark will come shortly and ...
OK, as far as RWLock is concerned, we totally agree.
Now I am less convinced by your approach of handling InterruptedException. It is not an error of any kind IMO, just a signal that interrupt() method has been called on the thread.
And interrupt() is not a deprecated Thread method (like stop() and supend() are), even if some book (but I didn't read it) discourage you to use it for any good or bad reason. I really don't know. My only purpose was to be "transparent" to the interrupt() and isInterrupted() methods. By throwing some IOInterruptedException, you force the caller to catch it (and BTW it has nothing to do with some IO exceptional condition), and you let the interrupted status cleared.
I really don't know what is the best approach. Maybe Max or Andrew ?
Regards,
Phil.

[Vlad Rabkin]

Hi Phil,

Now I am less convinced by your approach of handling InterruptedException. It is not an error of any kind IMO, just a signal that interrupt() method has been called on the thread.

It is not an error. As I said it can actually never happen, since I do not call interrupt method and do not need. (Probably in future if time-out functionality is desired it could be used, but not now for this assignement)
I pack it in the MyUnCheckedException, just because DB/DBAccess interface signatures do not allow it. I do the same think with all IOException (I remember our discussion. I decided not to use IORecordNotFoundException or any other kind of tricks: I wrap original exception in RuntimeException. My facade interface implementation catch it and throw an original checked exception to the client).
I just have corrected my previous message: I provided a sample code of Facade to be clear.

And interrupt() is not a deprecated Thread method (like stop() and supend() are), even if some book (but I didn't read it) discourage you to use it for any good or bad reason.

Whatever. Do you explicitely invoke this method somewhere? I don't, just because I don't needed it, since I do not have any time-outs.

I really don't know. My only purpose was to be "transparent" to the interrupt() and isInterrupted() methods. By throwing some IOInterruptedException, you force the caller to catch it (and BTW it has nothing to do with some IO exceptional condition), and you let the interrupted status cleared.

My only purpose was to be "transparent" to the interrupt() and isInterrupted() methods. By throwing some IOInterruptedException, you force the caller to catch it.

Exactly. I do understand "transparent" as follows: if something occured on the server it must be a client who decides what to do: try again, forget it and so on. "transparent" means to me client has a right to do know that something went wrong.
Let's take an example: you decide in future to build-in time-out functionality: after some time of waiting a time-out exception will be thrown. It is not the server responsibility to decide what to do further.
A real database server, which have connection time-out would do the same think: it would let the client know: your time is over.
So, it is absolutely correct to force the client to be prepared for such situation. By throwing IOInterruptedException I don't force him to handle it specially, since it has to handle anyway somehow IOExceptions.
If you don't agree with, could you give samples of the server behavier if this problem occurs? What should the server do: try again? how many times? Ignore it (I don't think it is good idea, because you need to acquire a lot to proceed with your read/write)?
Remember, all people who play with the time-out and thread interruption have been very hard penaltied.
Best,
Vlad
[ August 26, 2003: Message edited by: Vlad Rabkin ]

[Max Habibi]

Originally posted by Philippe Maquet:
Hi Vlad,

Yes I can imagine as I feel the same thing. You're right, let us congratulate us while we are just the two of us talking to each other : Max, Andrew or Mark will come shortly and ...
Phil.

I've been deliberately keeping out of this thread, because I knew you guys would work it on your own. So far, it looks pretty good to me. Even the points where you disagree with each other are based are good, careful analysis. My advice is to continue to argue it out. I'll step in if I think you're headed for a disaster.
All best,
M
[ August 26, 2003: Message edited by: Max Habibi ]

[Andrew Monkhouse]

Hi everyone,

[Philippe] Well, I disagree here with Andrew (it's so rare ! )

Good, I like it when people are willing to challenge things I say, and are willing to present arguments for their point of view.

Yes I can imagine as I feel the same thing. You're right, let us congratulate us while we are just the two of us talking to each other : Max, Andrew or Mark will come shortly and ...

[Philippe] my MultiReadSingleWriteSynchronizer class in 200 lines long : not a big deal !

200 lines is not a big deal? Once you have your MultiReadSingleWriteSynchronizer working correctly, then it will (hopefully) be easy to use and should need little or no maintenance. But it is 200 lines that a junior programmer will have to work through in order to maintain it. Or 200 lines that the examiner will have to read and understand in order to mark you on your locking code. That is 4 printed pages of code for them to read through just to understand an auxillary class to your main locking code.

[Max]: I've been deliberately keeping out of this thread

I was quite looking forward to the time when you would join in.
Perhaps soon Jim will join in and start convincing us all that his method is the best.
Regards, Andrew

[Jim Yingst]

Hi gang! My time is short, so I'm just going to comment on ThreadsafeDatabase1 again; y'all seem to have the others covered, and I haven't had time to look closely at any of them.
[Andrew]: As I see it, two separate threads could both try and update the same record at the same time. They could both get to your synchronized block at the same time. Thread A does the write to the database, exits the synchronized block, then gets swapped out. Thread B does it's write to the database and updates the cache and exits. Thread A then gets swapped back in, and updates the cache. Now we have the cache not matching what is in the database.
I think ThreadsafeDatabase1 has a few too many of these types of holes. Plus for any given user function, there may be several calls to internal functions which are synchronized. This will require a lot of overhead as locks are created and destroyed.
[Vlad]: Yeas, I agree with Andrew, because nobody knows what can occur between these two synchronized blocks, but I am not able to discuss it(neither advocate, nor critisize), since I don't know exactly Jims implementation details.
Yes, there are security holes, but that's not endemic to the design, just Vlad's implementation. One key point is that there's only one Record object for a given record number. When I do an update, I don't create a new Record and replace the old one - rather, I get the old Record, sync on it, and do everything I need to do with that record while inside the sync block. So it's something like:

Record record = getRecordFromCache(recNo);
synchronized (record) {
    record.checkIsUpdatable();  // throws exception if rec is deleted or cookie is invalid
    record.updateWith(newData);
    writeRecord(recNo, record);
}

So, there's no final putRecordInCache() method, because the Record is already in the cache. We just put new data into the Record. And we do that inside the record sync block, rather than outside.
Now, there's no way to prevent another thread from slicing in before the sync block, after the getRecordFromCache(). But so what? That's not really any different from something happening just before the getRecordFromCache() - because no matter what another thread does, the identity of the Record object will remain the same. Only its state may change. So once the sync lock is acquired, the first thing that needs to be done is check to see if the record is in a state that would allow the update. Has the record been deleted? (There's an isDeleted flag.) Then throw RecordNotFoundException. Has it been locked by another thread (with a different cookie value or whatever)? Or maybe it's not locked and it's supposed to be? Then throw a SecurityException. Otherwise, the record is eligible for update, and we've got exclusive access for the duration of the sync block.
As for Andrew's comments about overhead from excess synchronized calls. Well yes, that's theoretically possible, but in practice I've measured the effect of redundant synchronization here and found it to be negligible. Remember, these are uncontested locks. Once the first sync lock on a record has been acquired, it can be reacuired with relative ease. Most of the stigma that synchronization holds is from the time it takes to acquire heavily contested locks. (E.g., a sync lock on the whole DB, where each lock is held for a nontrivial amount of time.) My sync on the cache is more contested, but not much - since it's just an ArrayList, each cache lock is held for a very brief time. It's just not a big deal in practice.
[Vlad]:

        // What for should I synchronize cache.get(recNo):
        // Here before entering  synchronized (record) ...
        // Record Object can be changed by other thread !?

You sync on the cache because it's a mutable object shared among threads. Specifically, the create() method needs to be abble to add new elements to the cache, which is a structural modification, which means that any thread that wants to access the cache needs to synchronize if you don't want to run the risk of getting unpridictacle, unspecified results. This is separate from sync on the record - we need to know that we have the correct record first, before we sync on it. So we sync on the object that gives us the record.
[ August 27, 2003: Message edited by: Jim Yingst ]

[Philippe Maquet]

Hi Max,
Max
I've been deliberately keeping out of this thread, because I knew you guys would work it on your own. So far, it looks pretty good to me. Even the points where you disagree with each other are based are good, careful analysis. My advice is to continue to argue it out. I'll step in if I think you're headed for a disaster.
The only thing Vlad and I disagree is the way we should handle InterruptedException. But how to continue to argue it out ? Besides the fact that's probably not that much important, I think that both of us wrote everything he could about the subject.
Here is Vlad's code :

public synchronized void acquireReadLock()   
    throws InterruptedException {    
    ++waitingReaders_;    
    while (!allowReader()) {     
        try {      
            wait();     
        } 
        catch(InterruptedException ie) 
        {  
            --waitingReaders_; // roll back      
            throw ie;          
        }     
    }    
    --waitingReaders_;    
    ++activeReaders_;  
}

and my code :

    public synchronized void beginRead() {
        Thread caller = Thread.currentThread();
        while ( (!caller.isInterrupted()) && (locked || writing || locksAsked > 0 || 
                  writesAsked > 0)) {
            try {
                wait();
            }
            catch (InterruptedException e) {
                caller.interrupt();
            }
        }
        reads++;
    }

The main difference is that Vlad's method throws InterruptedException and swallows the interrupted status of the calling thread, while mine don't throw InterruptedException and keep the interrupted status unchanged.
The main reason for which I decided not to throw InterruptedException is simplicity of the calling code.
It's far more easy to code (and read) this :

try {
    beginRead();
    ...
    ...
}
finally {
    endRead();
}

than this :

boolean lockAcquired = false;
try {
    try {
        beginRead();
    }
    catch (InterruptedException e) {
        lockAcquired = false;
    }
    ...
    ...
}
finally {
    if (lockAcquired) {
        endRead();
    }
}

It could be simplified if Vlad had put a "++activeReaders_;" within its catch block, giving this as calling code :

try {
    try {
        beginRead();
    }
    catch (InterruptedException e) {
       // anything useful or nothing comes here
    }
    ...
    ...
}
finally {
    endRead();
}

Moreover, he swallows the interrupted status of the calling thread. Vlad argues that he never calls Thread.interrupt() in its own code (I do neither), but what if somebody else does ? Or even me in another context ? I put my MultiReadSingleWriteSynchronizer class in a suncertify.util package because I expect some reuse of it. I currently use two instances of it in the db package, but when you have such a class at hand you should use it to synchronize access to any collection (or any resource to be more general) for which you need synchronization and you reasonably expect to have much more reads than writes.
Max, Andrew, Jim, could you please put just an ounce of own opinion about it ? I know that this InterruptedException discussion has nothing to do with the main topic of this thread and I apology for this but as it's too late to put it somewhere else... Thank you in advance.
Andrew
200 lines is not a big deal? Once you have your MultiReadSingleWriteSynchronizer working correctly, then it will (hopefully) be easy to use and should need little or no maintenance. But it is 200 lines that a junior programmer will have to work through in order to maintain it.
I mentioned "including JavaDoc comments" but it's no more than 70 lines of real code. Better ? Well I believe - from experience - that the maintenance work for such a class is not far from zero.
Andrew
Or 200 lines that the examiner will have to read and understand in order to mark you on your locking code. That is 4 printed pages of code for them to read through just to understand an auxillary class to your main locking code.
I am afraid of that issue. Not for this class but for my whole suncertify.db package which is - you may suppose - quite huge (I even don't dare to tell you how much it is to avoid you to burst out laughing). I suppose - and hope so - that examiners don't read everything. If I was such a grader and if I had no other instructions, I would probably do the job as follows :

read the readme file

run my automated testing tool

run and discover the application

read some parts of its user guide

read choices.txt

choose parts of the source code and read it, parts which interest me, parts for which I may suspect issues from the choices.txt content and finally parts I choose randomly.

Does anybody (Max for sure) know how graders work ?
Max: I've been deliberately keeping out of this thread
Andrew:
I was quite looking forward to the time when you would join in.
Me too !
Best,
Phil.
[ August 27, 2003: Message edited by: Philippe Maquet ]

[Vlad Rabkin]

Hi guys,
Jim

ecord record = getRecordFromCache(recNo);
synchronized (record) {
    record.checkIsUpdatable();  
   record.updateWith(newData);
    writeRecord(recNo, record);} 
 

... So far so gut. If write in database fails, the cache his already updated. It is wrong. So, think it is must be like this:

Record record = getRecordFromCache(recNo);
synchronized (record) { 
   record.checkIsUpdatable();    
 writeRecord(recNo, newData);
 // update cache only if writing in DB succeeded
 record.updateWith(newData); 
   );}  

Jim, am I correct?

Phil, my synchronization block looks like this:

     try {
      rwManager.acquireWriteLock();
        try {
        data_.update(recNo, data, lockCookie);
        } finally {
            rwManager.releaseWriteLock();
        }
     } catch (InterruptedException ie) {
           throw new DatabaseSystemException(new IOInterruptedException("..."));
     }
    }

You are right, it is a bit ugly (My point was if I had all these things in a separate class it would be then Ok, but puting it all now to Data is ugly).
I don't know how Max is going to answer your question (He comes up always with extraordinary ideas), but this is a part of the code from his book:

    public boolean modifyDVD(DVD dvd) throws IOException {
        String upc = dvd.getUPC();
        try{
            boolean retval = false;
            db.reserveDVD(upc);
            retval =  db.modifyDVD(dvd);
            db.releaseDVD(upc);
            return retval;
        }catch(InterruptedException ie){
            log.throwing("DVDDbAdapter","returnRental("+ upc +")"
                                                            ,ie);
            InterruptedIOException iie =
                new InterruptedIOException("Threading Error");
            throw iie;
        }
    }  

Note, that reserveDVD, releaseDVD is something like lock/unlock, not beforeRaed, afterRead, but the idea is the same. As I have asked him what happens if modifyDVD fails (exception will be thrown, but DVD will not be released) he says nobody cares, it is enough to become J2CD...

I want to thank all of you : it seems that we have managed to make this topic active!
Vlad
[ August 27, 2003: Message edited by: Vlad Rabkin ]
[ August 27, 2003: Message edited by: Vlad Rabkin ]

[Philippe Maquet]

Hi Vlad,

Note, that reserveDVD, releaseDVD is something like lock/unlock, not beforeRaed, afterRead, but the idea is the same. As I have asked him what happens if modifyDVD fails (exception will be thrown, but DVD will not be released) he says nobody cares, it is enough to become J2CD...

Yes, it seems that it lacks some try .. finally out there. But if Max tells that "nobody cares" and that's enough to get SCJD, well it should be OK then.
Best, and thank you for this interesting discussion,
Philippe.

[Philippe Maquet]

Mmh... I am waiting for Mark telling us that try ... finally constructs are too hard to be understood by a junior programer. Keep it simple guys !
Sorry Mark and don't be angry on me, it was just a bad joke... In fact, I really appreciate most of your posts and they really help me to keep aware of the simplicity requirement (as far as I can ). And as I know so much that I need to be reminded to it regularly, thank you sincerely.
Best,
Phil.
[ August 27, 2003: Message edited by: Philippe Maquet ]

[Andrew Monkhouse]

Hi Everyone,

[Philippe]: If I was such a grader and if I had no other instructions, I would probably do the job as follows ... choose parts of the source code and read it, parts which interest me, parts for which I may suspect issues from the choices.txt content and finally parts I choose randomly.
Does anybody (Max for sure) know how graders work ?

I don't know for sure how the graders work.
But looking through the marks available and how they might be broken down (comparing the current marks to the old scoring system), I think you could be right except for one area: Locking. (and possibly server design).
Given how many marks are available for locking, and given that it can be difficult to physically prove that a locking design will fail, I would suspect that the examiner will manually inspect the locking code to see if they can find a logical place where the design could fail.
The same may apply for server design: they may manually inspect the code there to ensure that it is thread safe.
(Given how rapidly my assignment got marked, I am sure that the examiner did not inspect very much of my code - they probably just picked specific sections.)
Regards, Andrew

[Philippe Maquet]

Hi Andrew,
I think you're right about locking. It's probably a good place to go for manual inspection of our code (important stuff and not so huge).
For information, after ridding off all javadoc comments, my whole locking stuff (LockManager.java including LockManager class plus a few private nested classes (Connection extending WeakReference, Record, LockInfo and MaintenanceThread)) is just about 340 lines long. A quite readable volume, right ?

(Given how rapidly my assignment got marked, I am sure that the examiner did not inspect very much of my code - they probably just picked specific sections.)

If I remember, you just waited 48 hours to get the result, an amazing short time ! So yes, it's simply impossible to read everything so fast.
Regards,
Phil.