• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Synchronized Lock

 
ranger
Posts: 17347
11
Mac IntelliJ IDE Spring
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
OK, so how many times has this question been asked. But I still can't find a good answer that I like. So I am posting my lock code in the Data class, and the RemoteDataAccess class. I have tested two concurrent clients, and had one lock record #3 and loop for a few seconds, then I had the second client try to lock the same record, it waited until the first class unlocked the record before it could get a lock.
The only problem, is I have a while loop, instead of a wait()/notify(). Is this wrong?
*********lock method in my data class
public void lock(int record) throws DatabaseException{
while(lockedRecords.indexOf(new Integer(record)) < NOT_IN_LOCKED_RECORDS){}
lockedRecords.add(new Integer(record));
System.out.println(lockedRecords.toString());
}
}

******lock code in my RemoteDataAccess class
public void lock(int record) throws RemoteException, DatabaseException{
int recordIndex;
if (record == LOCK_ALL_RECORDS){
for(int loopCount = 0; loopCount < getRecordCount(); loopCount++){
lock(loopCount);
}
recordIndex = lockedRecordIndexInVector(record);
} else {
recordIndex = lockedRecordIndexInVector(record);
if (recordIndex == NOT_IN_LOCKED_RECORD){
FBNDatabase.lock(record);
lockedRecords.add(new Integer(record));
}
}
}

lockedRecords is a Vector.
Is my code threadsafe, or a big risk?
Thanks
Mark
 
author
Posts: 3252
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Originally posted by Mark Spritzler:
The only problem, is I have a while loop, instead of a wait()/notify(). Is this wrong?

Quite simply, yes. On machines without pre-emptive threads, your implementation will sooner or later hang because one thread keeps on hogging the CPU (you may fix that by incorporating a yield() in the while loop). Furthermore, it wastes large amounts of perfectly good CPU time by sitting in a tight loop like that.

Is my code threadsafe, or a big risk?

The code is not threadsafe. Take another look at it:
There are obvious problems here, for a start the braces don't match up, but I assume you meant<br /> Because there is no synchronization, nothing prevents another thread from grabbing the lock just between your while and your add, causing you to grant the same lock twice. The risk may be small but it is a major bug. You may be selling the same seat twice.
There are other, less important problems.
Using contains() rather than indexOf() would be a lot clearer.
The use of indexOf tells me you are using some type of List, probably an ArrayList or a Vector. Characteristic of these are that they are ordered and allow duplicate elements. However, for the locks you are not interested in the order and you do not want any duplicate elements! What you need is not a List, but a Set, probably a HashSet. It is no coincidence that it will be a lot faster too (O(1) instead of O(n)).
Finally, the while() loop above will generate huge amounts of object churn because it keeps on creating Integer objects; this will flood the garbage collector and greatly slow down your server. Create a single Integer at the start of the method and keep on using that.
- Peter

[This message has been edited by Peter den Haan (edited October 01, 2001).]
 
Mark Spritzler
ranger
Posts: 17347
11
Mac IntelliJ IDE Spring
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Again Peter you come through in the clutch.
I was reading in the O-Reilly book "Learning Java" and I read about thread and wait and notify. I think I will synchronize the "Vector", if it remains a Vector. Then in the loop, it will have a wait(). In the unlock method I will synchronize the same object, and when it is done unlocking it will call notifyAll().
In this case can I keep the Vector. Yes I know it allows duplicate values, but all it takes is one object, the Integer of the record number, whereas a HashMap will need two objects, the Key and the Object. However, thinking about it, Key is the Integer.toString(), and the object is the Integer, and then I can just use contains(TheInteger).
Thanks again Peter
Mark
 
Mark Spritzler
ranger
Posts: 17347
11
Mac IntelliJ IDE Spring
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
One more thanks Peter. I made the changes and used a HashSet instead, and I tested it with two JVM's A & B trying to lock the same record, A first runs and gets the lock on the record, it then loops for a few seconds, in which I run B, which is trying to lock the record for the record that is locked by A. It waits, and when A unlocks and it notifies all, then B gets the lock. All really cool, and it only took me ten minutes to change what I had, to what you where suggesting.
HashSet is much easier with less code.
Thanks
Mark
 
reply
    Bookmark Topic Watch Topic
  • New Topic