Tests for the Data class/locking mechanism

[Roel De Nijs]

Olu Shiyan wrote:the 10,000,000 iterations took less than a minute to run

That's quiet impressive and a really performant solution! (I remember mine being a whole lot slower, the drawback of using synchronized methods with wait/notifyAll)

[edit] woohoo, 1st post on a new page

[Olu Shiyan]

Roel,

C'mon Roel, you got a perfect score man.

I also used wait/notifyAll and synchronized all my public methods. With regards to the running time, I think it depends on the size of the database file and the computer hardware. For example, 10,000,000 iterations runs in about 25 seconds the first time I run it, it takes more time the second time I run it but never more than a minute. My PC specs are quite high though.



A question for you though: I used a record cache but decided to write to both my record cache and file during write operatons. So for example, My create method inserts the newly created record into the cache and also inserts it in the appropriate location in the database file (using RandomAccessFile's seek method), my update and delete methods follow a similar approach.

What do you think about this?



Cheers

[Roel De Nijs]

Olu Shiyan wrote:C'mon Roel, you got a perfect score man.

Don't forget: high performance solutions will not score better than low performance ones, because performance is not an actual requirement. I just remember I used 20000 iterations as maximum (could also have been 100000, long time ago ) because it was so slow. So 10 million in less than a minute is really impressive (but maybe my hardware is a bit outdated)

I think it's not the appropriate topic to start a discussion about that

[Olu Shiyan]

Don't forget: high performance solutions will not score better than low performance ones, because performance is not an actual requirement

Yes you're right and I didnt specifically design a high performance solution; I used a cache like you and loads of other people did. Nothing special.

I think it's not the appropriate topic to start a discussion about that

my second question has been moved here




Thanks

[Robert Benson]

I've got my test to run with 10,000+ tests without issue.

However, I'm not using a locking map (dont think there is a requirement in my assignment for one), instead, I've synchronized a code block.

sychronized(this) {
lock(recNo);
update(...., ....);
unlock(recNo,cookie);
}  

My reasoning is, this seems cleaner and more maintainable that using a locking map. The locking map requires among others:
1/ a decalaration of a reentrant lock
2/ a lock/unlock before and after the map update in the lock() method using a try/finally block
3/ a lock/unlock before and after the map update in the unlock() method using a try/finally block

Thats a lot more code to do the same thing. Is a synchronized block acceptable or should I implement the locking map?

Thanks, Robert.

[Piotr Nowicki]

Howdy Robert!

Are you using a singleton pattern for data class? If so, your atomicity of operations are not on lock/unlock/update level but rather on all those operations together, right?
Do I understand you correctly, that in your case, such a scenario can never occur:

T1: locks recNo 4
T2: locks recNo 6
T1: updates recNo 4
T1: unlocks recNo 4
T2: unlocks recNo 6

You will get only serial execution of T1, then T2 and so on, like:

T1: locks recNo 4
T1: updates recNo 4
T1: unlocks recNo 4
T2: locks recNo 6
T2: unlocks recNo 6

The other possible case is that your data class is not a singleton. If so, there are multiple instances of this class, so synchronizing on 'this' won't give you any thread-safety at all.

Don't really know what is your assignment, but as far as I understand - this is an undesired state - why the lock/unlock method then at all?

AD1. You do not need a reentrant lock to implement the locking map.

I don't quite follow points 2 and 3 however...

[Roel De Nijs]

Hi Robert,

I fully agree with Pedro: your approach doesn't allow concurrency at all. Methods lock and unlock are completely useless with this approach. A scenario shown by Pedro (with a non-sequential output) should be possible when using the Data class.

Kind regards,
Roel

[Robert Benson]

Thanks Pedro and Roel.

When you explain it like that, its obvious. Now I understand.

While the instructions do not ask specifically for a locking map, you must implement one for concurrency.

[Sean Keane]

Just ran this class and it helped me iron out a few bugs, many thanks Roberto!

One Change

The only change I had to make was due to my interface using cookies, so where Roel had something like this:

data.lock(recNo);
data.update(recNo, RoomRetriever.asArray(room));
data.unlock(recNo);

I replaced it with:

long someCookie = data.lock(recNo);
data.update(recNo, RoomRetriever.asArray(room), someCookie);
data.unlock(recNo, someCookie);

One Suggestion

To aid diagnosing problems I changed the constructor for the threads to take an argument

for (int i = 0; i < 10; i++) {
        Thread updatingRandom = new UpdatingRandomRecordThread("Thread-UpdateRandomRecord-" + i);
        updatingRandom.start();

Then in the contructor I set the thread name to the argument passed in

private class UpdatingRandomRecordThread extends Thread {

    public UpdatingRandomRecordThread(String string) {
      this.setName(string);
    }

Anywhere Roberto had Thread.currentThread().getID() I replaced it with Thread.currentThread().getName(). I found this output more intuitive and easier to see what was going on.

Then in my Data class I added logging that made use of the fact that the thread name was set e.g.:

public synchronized long lock(final int recNo) throws RecordNotFoundException {

// do the work to lock the recod

// log the fact that this record is now locked
System.out.println("LOCKED: record=" + recNo + " cookie=" + cookie
      + ", thread=" + Thread.currentThread().getName());
}

[Roel De Nijs]

Sean Keane wrote:so where Roel had something like this:

It finally happened: I got mixed up with the great and wise Roberto Perillo

[Sean Keane]

Roel De Nijs wrote:

Sean Keane wrote:so where Roel had something like this:

It finally happened: I got mixed up with the great and wise Roberto Perillo

Oooops . I thought I'd replaced all my references to Roel with Roberto in my update - after I realised it was Roberto who had provided us with this wonderful test class! Your equal brilliance makes you guys interchangeable anyhow ;-)

[Roberto Perillo]

Sean Keane wrote:Your equal brilliance makes you guys interchangeable anyhow ;-)

I'm delighted!

[Sean Keane]

I run this test as part of my suite of unit tests and I came across something that confused me - then I figured out what was going wrong. So I'm sharing here in case others come across this and mistakenly think they have deadlock in their solution.

I was making some changes to my Data class, but not to the locking mechanism, then all of a sudden I was getting what seemed like deadlock in this test - the test would just hang there, failing to terminate. After comparing my Business class (where I use my locking API) and this DataClassTest I figured out what the problem is.

The problem is with this the test of the update method in the DataClassTest class. This is the code here:

try {
        long someCookie = data.lock(1);
        data.update(1, RoomRetriever.asArray(room), someCookie);
        data.unlock(1, someCookie);
      } catch (Exception e) {
        System.out.println(e);
      }

I was seeing seeing what appearing like deadlock when running the DataClassTest simply because the following was happening in DataClassTest when testing the update method of the Data class:

1. It got the lock
2. It attempted to update the record, but this failed with an Exception being thrown (this was an error in my code causing the Exception to be thrown)
3. It then jumped to the catch block

This sequence of events meant that the record was never unlocked. So other threads that were waiting to lock this same record would simply hang forever - so no deadlock in my solution phew .

To replicate this problem, simply replace the code snippet above with this snippet:

try {
        long someCookie = data.lock(1);
        data.update(1, RoomRetriever.asArray(room), someCookie);
        throw new Exception("update exception");
        data.unlock(1, someCookie);
      } catch (Exception e) {
        System.out.println(e);
      }

In my Business class I don't experience this problem as I have put the call to unlock into a finally block. Something like this:

long cookie;
try {
  //lock record
  cookie = db.lock(...);
} catch (RecordNotFoundException e){
  //handle exception
}

try {
  db.update(...);
} catch (RecordNotFoundException e) {
  //handle exception
} finally {
  //handle any exceptions this method throws
  db.unlock(...);
} 

Maybe this would be a good update to make to the DataClassTest?

EDIT-1: Updated with pseudo code instead of working code. But if you are to change the DataClassTest to handle this situation correctly you will have to update the test class with working code that is pretty much similar to what you have in your business class - as they are doing the same thing, hence why I originally posted the working code (which is not 100% my code from my business class).

[Sean Keane]

Here's my update to the DataClassTest, might be useful:

  private class UpdatingRandomRecordThread extends Thread {

    public UpdatingRandomRecordThread(String string) {

      this.setName(string);
    }

    @Override
    @SuppressWarnings("deprecation")
    public void run() {

      final Room room = new Room();
      room.setName("Palace");
      room.setLocation("Smallville");
      room.setSize(2);
      room.setSmoking(true);
      room.setRate("$150.00");
      room.setDate(new Date(2005, 06, 27));
      room.setOwner("54120584");

      final int recNo = (int) (Math.random() * 50);

      // setting this to -1, but if you rethrow the exception in the catch block associated with the 
     // lock() method, then you won't need to assign a value to the someCookie variable.
      long someCookie = -1;

      try {
        System.out.println(Thread.currentThread().getName()
          + " trying to lock record #" + recNo
          + " on UpdatingRandomRecordThread");

        someCookie = data.lock(recNo);
        System.out.println(Thread.currentThread().getName()
          + " trying to update record #" + recNo
          + " on UpdatingRandomRecordThread");
      } catch (Exception e) {
        System.out.println("THREAD = " + Thread.currentThread().getName()
          + "\tRECORD=" + recNo + "\t" + e);
        e.printStackTrace();
        // we should rethrow the exception here, as we don't want to continue on
        // to update the record as we have not been able to lock it, but just
        // preventing it continuing for now.
        return;
      }

      try {
        data.update(recNo, RoomRetriever.asArray(room), someCookie);
      } catch (Exception e) {
        System.out.println("THREAD = " + Thread.currentThread().getName()
          + "\tRECORD=" + recNo + "\t" + e);
        e.printStackTrace();
      } finally {
        try {
        System.out.println(Thread.currentThread().getName()
          + " trying to unlock record #" + recNo
          + " on UpdatingRandomRecordThread");
          data.unlock(recNo, someCookie);
        } catch (RecordNotFoundException e) {
          System.out.println("THREAD = " + Thread.currentThread().getName()
            + "\tRECORD=" + recNo + "\t" + e);
          e.printStackTrace();
        } catch (SecurityException e) {
          System.out.println("THREAD = " + Thread.currentThread().getName()
            + "\tRECORD=" + recNo + "\t" + e);
          e.printStackTrace();
        }
      }
    }
  }

[Jonathan Elkharrat]

Roel De Nijs wrote:Hi Robert,

I fully agree with Pedro: your approach doesn't allow concurrency at all. Methods lock and unlock are completely useless with this approach. A scenario shown by Pedro (with a non-sequential output) should be possible when using the Data class.

Kind regards,
Roel

but that's the asked behavior. T2 should go to wait state until T1 is finished.

besides, the lock and unlock are useful if you do more than one operation. (e.g. book() should do
consecutive read() and update() and must be sure no one wrote to the database between them)

[Roel De Nijs]

Jonathan Elkharrat wrote:

Roel De Nijs wrote:Hi Robert,

I fully agree with Pedro: your approach doesn't allow concurrency at all. Methods lock and unlock are completely useless with this approach. A scenario shown by Pedro (with a non-sequential output) should be possible when using the Data class.

Kind regards,
Roel

but that's the asked behavior. T2 should go to wait state until T1 is finished.

That's not true at all! T2 should only go to wait if it wants to lock the same record as T1. If T1 locks record 5 and T2 locks record 2, then no thread has to go to waiting state.

[Jonathan Elkharrat]

oh, sorry, i didn't notice he's synchronizing the whole server function.
i think i wasn't fully awake this morning.
my sincere apologies...

[Sean Keane]

Was confused about these last three updates after my post about locking in the test class. But I see now, they relate to a post on this thread from almost a year ago.

[Ixus See]

how did you guys manage to run 1000x times with no problem?

I can only manage to get up to 100 times.. Is there something wrong with my code ;(

[Roel De Nijs]

In order to get some useful tips and hints, you must provide a lot more information. Just showing the modified DataClassTest is useless, because we all know how that looks like.

You say you can only increase counter to 100. What happens with your program if you increase the counter to 500 or 1000? Giving us such information is much more important than the modified DataClassTest.

[Roberto Perillo]

Roel De Nijs wrote:You say you can only increase counter to 100. What happens with your program if you increase the counter to 500 or 1000? Giving us such information is much more important than the modified DataClassTest.

Agreed. For instance, do you run into a deadlock? That's weird, because that would (could) happen when running the program 100x as well.

[Miguel Angel Gonzalez]

Hey Roberto,

Just want to thank you, your test class is really useful. I also had a heart attack when my application faced a deadlock, but checking my code I found that I had a call to 'lock()' method when I was actually trying to release it, so just changed it to 'unlock()' and all went smooth even for 1000 iterations.

Thanks again

[Roberto Perillo]

Alright, champion! I'm very glad these tests were helpful!

[Sophie Siegel]

Hallo,

thank you for sharing your testclass!

I tried it with 10 and all runs fine. I checked the file and everything was done correct.

 for (int i = 0; i < 10; i++) {  
                 Thread updatingRandom = new UpdatingRandomRecordThread();  
                 updatingRandom.start();  
                 Thread updatingRecord1 = new UpdatingRecord1Thread();  
                 updatingRecord1.start();  
                 Thread creatingRecord = new CreatingRecordThread();  
                 creatingRecord.start();  
                 Thread deletingRecord = new DeletingRecord1Thread();  
                 deletingRecord.start();  
                 Thread findingRecords = new FindingRecordsThread();  
                 findingRecords.start();  
             }  

If I tried it with 100000 it also terminated. But if I tried this without the DeleteThread there must be 100000 new items in my file.
But there are only 1432

Sometimes I see this exception:

Exception in thread "main" 1135 trying to lock record #1 on UpdatingRecord1Thread
1134 trying to lock record #20 on UpdatingRandomRecordThread
java.lang.OutOfMemoryError: unable to create new native thread
1133 trying to find records
1131 trying to create a record
at java.lang.Thread.start0(Native Method)
at java.lang.Thread.start(Unknown Source)
at test.DataClassTest.startTests(DataClassTest.java:79)
at test.DataClassTest.main(DataClassTest.java:66)

Should I now worry or is this because of my computer don't have so much memory?

Regards,
Sophie

[Roel De Nijs]

Sophie Siegel wrote:Should I now worry or is this because of my computer don't have so much memory?

Here is a whole thread about this topic

[Andre Roodt]

Thanks for this test Roberto, you saved my skin with a careless error on my side. I wasn't unlocking the record when exceptions were being thrown in the Data class causing deadlocks to occur. I wasn't sure whether to fix this in the data class or in the code that calls the method in the data class. I eventually decided to fix it in the Data class where I handle the exception for the sake of the exam criteria stating that the data class must not cause deadlocks.

Anyway, thanks for the effort in providing this test.

[Roel De Nijs]

Andre Roodt wrote:I wasn't sure whether to fix this in the data class or in the code that calls the method in the data class. I eventually decided to fix it in the Data class where I handle the exception for the sake of the exam criteria stating that the data class must not cause deadlocks.

In my opinion that's not the right place to solve the issue! You should simply put the code that calls Data class methods in a try(-catch)-finally and unlock the record in the finally-block. That's correct use of the API (Data class). Making the Data class responsible for this multi-threaded issue is an example of bad design.

Just my 2 cents.

[Andre Roodt]

Roel De Nijs wrote:

Andre Roodt wrote:I wasn't sure whether to fix this in the data class or in the code that calls the method in the data class. I eventually decided to fix it in the Data class where I handle the exception for the sake of the exam criteria stating that the data class must not cause deadlocks.

In my opinion that's not the right place to solve the issue! You should simply put the code that calls Data class methods in a try(-catch)-finally and unlock the record in the finally-block. That's correct use of the API (Data class). Making the Data class responsible for this multi-threaded issue is an example of bad design.

Just my 2 cents.

I totally agree with you. I'm just concerned that as an entity on its own, the data class can deadlock if the calling code doesn't handle the exceptions correctly(i.e. unlocking in the catch or finally). Will this then constitute a failure in the eyes of the examiner? I'm just wary. I guess I could explicitly document the method saying the caller of the method should be sure to unlock the record in the event of an exception occurring.

[Andre Roodt]

Sorry guys, i didn't read the whole thread. I see this issue has been discussed before.

[Bj Peter DeLaCruz]

I modified Robert's code and used a thread pool of 500 threads to update, delete, create, and find records.

I used a latch with an initial count of 40,000 and iterated 40,000 times (10,000 times for each of the four operations above). Then I set the main thread to wait 3 minutes. If the count is zero, then there was no deadlock. Otherwise, if the timeout is exceeded and the count is not zero, then there was a deadlock.

It took 36 seconds for my test to complete. If anyone is interested in testing the locking mechanism in the Data class using my test code, I can post it here or give you a link to my Github account where I host it.

[Roel De Nijs]

Bj Peter DeLaCruz wrote:If anyone is interested in testing the locking mechanism in the Data class using my test code, I can post it here or give you a link to my Github account where I host it.

You can always share test code, it's be highly appreciated by other OCMJD candidates. But sharing your actual OCMJD code isn't allowed, so please be careful with sharing your GitHub account.

[Bj Peter DeLaCruz]

I only posted my test code on Github, but thanks for the heads up.

[Robin van Riel]

Hi Peter,

Apart from finishing up my documentation the only thing I still need to do before handing in my assignment is making absolutely sure my locking mechanism is working as expected.
The more tests the merrier .

So if you would still be willing to share you're locking test, it would be very welcome.

Cheers,
Robin.