permaculture playing cards*
The moose likes Developer Certification (SCJD/OCMJD) and the fly likes is this lock&unlock ok? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Murach's Java Servlets and JSP this week in the Servlets forum!
JavaRanch » Java Forums » Certification » Developer Certification (SCJD/OCMJD)
Bookmark "is this lock&unlock ok?" Watch "is this lock&unlock ok?" New topic
Author

is this lock&unlock ok?

Sean Li
Ranch Hand

Joined: Feb 27, 2002
Posts: 154
public class FlightModelDbImpl extends UnicastRemoteObject implements FlightModel{
private static Hashtable lockManager=new Hashtable();
......
public synchronized void lock(int record) throws Exception {
if (lockManager.containsKey(new Integer(record))) {
try{
this.wait();
}catch(Exception e){
}
}else{
lockManager.put(new Integer(Thread.currentThread().hashCode()),new Integer(record));
}
}
public synchronized void unlock(int record) throws Exception{
Integer aa=(Integer)lockManager.get(new Integer(Thread.currentThread().hashCode()));
if (aa.intValue()==record) {
lockManager.remove(aa);
this.notifyAll();
}
}
Ramesh kumaar
Ranch Hand

Joined: Mar 19, 2002
Posts: 146
Hi,
I think there is no need to synchronize the entire lock and unlock method to be synchronized instred use synchronized blocks on the hashtable
where the real concurrency comes into picture.
Hope this helps
-rameshkumar
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252
The naming of this class and the interface suggests that they are specific to the Fly By Night project. Is that so, and is that wise?
Code to interface, not implementation! Make lockManager a Map. Also, Hashtable is from the old JDK 1.1 classes; use a proper Collections class such as HashMap. The API is much cleaner, and you don't pay the performance penalty of synchronisation you don't need.
This code has several problems. First of all, it doesn't work; the "if" needs to be a "while". Second, consider creating the record Integer only once at the start of the method. Third, the completely amorphous "throws Exception" clause is really bad practice; you'd be well advised to clean up your exception act. Last but not least, you are using the current thread to identify the client. The RMI specification explicitly does not guarantee any particular mapping between threads and method calls, so while this happens to work with the current RMI implementation, it may be broken by another vendor's JDK or future versions of the Sun JDK.
The same remark about throwing exceptions applies here, really. And surely you can think of a more meaningful variable name than "aa"?
Your synchronisation is fine, by the way. I disagree with Ramesh Kumaar's comment. Synchronized blocks are harder to read and wouldn't buy you anything significant in terms of concurrency.
- Peter
Ramesh kumaar
Ranch Hand

Joined: Mar 19, 2002
Posts: 146
Hi Peter,

As u said if ur going to synchronize the method instred of a block then there is no need to implement wait() and notify(). Since u synchronize the entire method that allows only one request to be processed at time and all the remaining has to wait till the current request get completed, where i feel like concurrency is lost.
-rameshkumar
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252
Originally posted by Ramesh kumaar:
As u said if ur going to synchronize the method instred of a block then there is no need to implement wait() and notify().
But I didn't say that -- there is every reason to!
Since u synchronize the entire method that allows only one request to be processed at time and all the remaining has to wait till the current request get completed, where i feel like concurrency is lost.
That's not how it works. First of all, these methods are only called to set or release a lock. Their execution time is generally very brief, except when lock() blocks because a lock cannot immediately be acquired. But even then, concurrency is preserved because the wait() call releases the monitor lock held by the thread. This frees up the lock manager for other calls.
Besides, the containsKey/put sequence in lock() needs to be atomic and therefore synchronized as a whole. You must synchronize the entire method or you're dead in the water. Even if you are using a synchronized Map (in that case you'd have to synchronize on the Map using a synchronized block, making the code harder to read).
The calls to get() and remove() in unlock() need to be synchronized as well (if you're using an unsychronized Map). You can put these calls in separate synchronized blocks, but that would double the number of lock acquisitions in return for no significant increase in concurrency, which is almost certain to be a net performance loss. Using a sychronized Map is equivalent to using two synchronized blocks.
- Peter
[ December 18, 2002: Message edited by: Peter den Haan ]
Sean Li
Ranch Hand

Joined: Feb 27, 2002
Posts: 154
Thanks to your reply, indeed.
while I encountered another question: how to implements the lock method if the recno parameter is -1, just add a (clientId, -1) to the hashMap or add every record in the database into the hashMap?
if I add just -1 as a whole to the hashMap, if that means i have to unlock it as a whole too, or can i unlock it one by one?
if I add all records to the hashMap(for instance, 24 records), so i can unlock it one by one, then what will happen such thing happen:
1, Thread A invoke lock(1)
2, Thread B invoke lock(-1)
3, Thread B invoke unlock(-1)
so? in this situation, unlock record 1 or not?? I think not. but how to implement this?
waiting for your further reply and really thanks.
Sean Li
Ranch Hand

Joined: Feb 27, 2002
Posts: 154
And I'm confused now, by your instruction, I create a new object for each remote client. But how can I pass the object to the lock method? it has only one parameter: recno.
could u help me?
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252
Originally posted by Sean Lee:
And I'm confused now, by your instruction, I create a new object for each remote client. But how can I pass the object to the lock method? it has only one parameter: recno.
The lock() method in DataInterface has just one parameter. The lock() method in your lock manager must have two. So your Connection (or RemoteDataInterfaceImpl or whatever) might look something like the followingAnd the LockManager class might contain a method likeNote that I've secretly inverted your map; the locks map in this example maps record numbers to their owner instead of vice versa. There are two very good reasons for this.
  • Remember that the keys in a HashMap are a Set, meaning that no duplicates are allowed. Making the clients a Set means that each client can hold just one lock, which is a serious and unnecessary restriction. Making the records a Set means that each record can be locked just once, which is just fine, in fact this is a fundamental constraint that is at the root of the locking system.
  • Remember that a Map is very efficient (O(1) in the case of a HashMap) at finding the value for a given key. In lock() and unlock(), you need to find out the owner for a given record lock. With the Map the wrong way around this is an O(n) operation. In my code, this is an efficient O(1) operation.
  • Make sure you know the Collections framework inside out, and try to map constraints in your problem to constraints of your collections. You will usually find that this automatically guides you towards an optimal model.
    - Peter
    [ December 18, 2002: Message edited by: Peter den Haan ]
    Mark Spritzler
    ranger
    Sheriff

    Joined: Feb 05, 2001
    Posts: 17249
        
        6

    Sean, lock(-1) can be assumed to be called only when the server wants to close.
    You will loop though all the records and recursively call lock with each and every record.
    You do not need to code for unlock(-1) as the server will close itself after it has attained all the locks, and is no longer waiting for any clients to unlock their records.
    Mark


    Perfect World Programming, LLC - Two Laptop Bag - Tube Organizer
    How to Ask Questions the Smart Way FAQ
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Originally posted by Mark Spritzler:
    Sean, lock(-1) can be assumed to be called only when the server wants to close.
    Cute. Clearly this is one of the areas where the interpretation of the requirements may differ; I interpreted as a kind of poor man's table lock and implemented both lock(-1) and unlock(-1). Your point of view would've saved me some work
    - Peter
    Mark Spritzler
    ranger
    Sheriff

    Joined: Feb 05, 2001
    Posts: 17249
        
        6

    You know Peter, the interesting part of it all, is that in the requirements it discusses lock(-1), but never mentions unlock(-1).
    But I am always open to others interpretation. Because I have been known to be wrong a good number of times. Just ask my boss.
    Mark
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Yes, that's absolutely true, Mark. I just never looked at it that way...
    - Peter
    Ramesh kumaar
    Ranch Hand

    Joined: Mar 19, 2002
    Posts: 146
    Hi All,
    ------------------------------------------------
    orginally posted by Mark:
    You will loop though all the records and recursively call lock with each and every record.
    ------------------------------------------------
    I think there is no need to recursively call lock with each and every record.
    The main idea of lock(-1) is to make sure the server shutdown happens smoothly. ie the existing request are completed before the server shutdown, and once lock(-1) called no new request are allowed get processed, here a message should be send to the client saying "Server is under maintanance" instered making it to wait unnecesserly. Here i feel we need to do some thing other than recursively calling lock with each and every record.
    -rameshkumar
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Actually I think both solutions have pitfalls. Remember, your server is reusable, and might be used in applications that require multiple simultaneous locks.
    Progressively locking all your records may easily lead to deadlock if a client managed to get its first lock and contends with the lock(-1) for the second. Refusing to process any more requests can lead to the same problem, unless lock() throws an exception when a full database lock is in progress.
    My own solution was to have a lock(-1) wait passively until a point where all record locks had cleared. No risk of deadlock, but the price to pay is that you may have to wait a while for your database lock.
    Which solution is the best? Well, that entirely depends on how you interpret the lock(-1) functionality, what you think it will be used for.
    - Peter
    Sean Li
    Ranch Hand

    Joined: Feb 27, 2002
    Posts: 154
    Thanks to your reply, now I've done my work in lock and unlock.
    And I just did what u told me to write a Factory class to create remote data access instance for every client.
    After I finished it, I found it cannot work. the Factory can be gained from the registry, but cannot use its getConnection method. the exception is notSerializableException.
    I guess it's because the Factory is a UnicastRemoteObject so it can be gained and used. but the Connection object is not. so when I try to get the Connection instance the JVM will tell me I cannot use it because the reason stated above.
    I don't know if I'm right, just keep helping me, buddies!
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Make Connection extend UnicastRemoteObject, or simply export it explicitly by calling the (static) UnicastRemoteObject.exportObject() method -- be sure to read the javadoc though in that case.
    - Peter
    [ December 19, 2002: Message edited by: Peter den Haan ]
    Sean Li
    Ranch Hand

    Joined: Feb 27, 2002
    Posts: 154
    Thanks peter, after test, I know one thing: if the client side want to use one object in the remote server through rmi, the remote object must extend UnicastRemoteObject, and must use rmic to compile it to genarate stub and skel files.
    while I feel the structure is very complicated now. the RemoteDataImpl implements RemoteData interface to serve the client. to use MVC pattern in this assignment, I wrote a Model interface and two classes to implement it: ModelLocalImpl and ModelRemoteImpl, but at last I found all this classes( ModelLocalImpl,ModelRemoteImpl,RemoteDataImpl,Data...) have the same functions, the only differences are the implementation and Exceptions they throw. why not just use one interface to fascade all data functions? I don't know if that is ok and more clear.
    I know it's an OO design issue, but I'm not very clear about the structure. any help is appricate!
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Originally posted by Sean Lee:
    to use MVC pattern in this assignment, I wrote a Model interface and two classes to implement it: ModelLocalImpl and ModelRemoteImpl
    Hahaaa! I sense an opportunity to whip out my world-famous patented 3-tier architecture boilerplate diagram[tm].
    UI <==> business logic <==> database
    Three layers, two interfaces. A few observations.
  • In a clean design, your RMI interface between client and server will correspond to one of these two interfaces. So there are two possible choices. They are mutually exclusive -- you can't have both without mixing up layers which is a bad idea in any system.
  • The database layer in this assignment is, effectively, Data and the networked stuff you put on top of it. So the Data API, the public methods in Data, is the business logic <==> database interface.
  • Your Model interface corresponds to the UI <==> business logic interface.
  • The instructions dictate that you must have a client-side class implementing all the public methods in Data. In other words, the database interface must be exposed at the client side.
  • The database interface must be exposed at the client side. That makes no sense at all if your RMI interface corresponds to the UI <==> business logic split. On the other hand, it is satisified automatically if your RMI interface corresponds to the business logic <==> database split. So the Sun requirements effectively tell you that the business logic should reside on the client.
    With your Model interface, you're trying to put the business logic on the server.
    Don't. You can still implement the MVC pattern but it would reside entirely within the client application.
    - Peter
    [ December 20, 2002: Message edited by: Peter den Haan ]
    G.T. Reddy
    Ranch Hand

    Joined: Jun 25, 2002
    Posts: 45
    --------------------------------------------------
    public synchronized void lock(Object owner, int record) { try { Integer recordKey = new Integer(record); while (locks.containsKey(recordKey)) { this.wait(); } locks.put(recordKey, owner); } catch (InterruptedException e) { throw new RuntimeException("Interrupted!", e); } }
    Note that I've secretly inverted your map; the locks map in this example maps record numbers to their owner instead of vice versa. There are two very good reasons for this
    Remember that a Map is very efficient (O(1) in the case of a HashMap) at finding the value for a given key. In lock() and unlock(), you need to find out the owner for a given record lock. With the Map the wrong way around this is an O(n) operation. In my code, this is an efficient O(1) operation.
    --------------------------------------------------
    Hi peter,
    I am not able to understand the sentences which are written in bold.
    could you please elaborate them.
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Remember that a Map is very efficient (O(1) in the case of a HashMap) at finding the value for a given key.
    This means that for a HashMap, a call to map.get(key) takes a more or less constant amount of time, regardless of how many elements there are in the map. In other words, its performance does not deteriorate when things get tough.
    In lock() and unlock(), you need to find out the owner for a given record lock. With the Map the wrong way around this is an O(n) operation. In my code, this is an efficient O(1) operation.
    This revolves around the question, what do you need to do to find out who the owner of a given lock is?
    Assume for the moment that your Map keys are the lock owners, and each value is the record locked by the corresponding owner. This is how the original code worked. As a Map does not have a convenient method to find a given value, you are reduced to going through the values one by one. On average, you have to search through half the values in your map before you have discovered the record you were looking for. The time taken is proportional to the number of elements (values) in the map, which is notated as O(n).
    This is very bad behaviour. The number of locks present at any given time -- n -- will be proportional to the number of clients (let's call this N). At the same time, the number of lock checks per second will be proportional to the number of clients as well. So the total CPU load imposed by the locking code will be O(N*N). Ten times more clients means a hundred times more time spent in the locking code. N-squared behaviour is usually a sign that you're doing something suboptimally or just plain wrong, and certainly a sign that what you're building is going to break down under load. While performance is not a prime consideration in the assignment, suboptimal implementation is.
    Now assume the map works the other way around: the keys are the record numbers, and the value for each key is the owner of the lock (if any). A simple call to map.get(recordNumber) will tell you who the owner of the given record lock is. This takes a constant, O(1) time, regardless of how many locks there are. This is much better behaviour: the total CPU time spent in the locking code will be O(N), which is as good as things generally get.
    A completely different slant on the problem is the mathematical, set-theoretical one. The keys of a Map form a Set (no duplicates allowed). The values of a Map form a Collection (duplicates allowed).
    You cannot lock a record more than once, which means that the locked records form a Set. There is no reason why a client should not be able to lock more than one record, so lock owners may be duplicated: they form a Collection.
    These considerations alone steer you towards mapping record numbers to lock owners.
    Finally, you can look at this from a functionality point of view. If you define the map the wrong way around, you pay two prices: (1) the crippling restriction that a client cannot own more than one lock at a time and (2) your data structure does not prevent a lock from being owned by more than one client, you will have to ensure this in your own code. This opens up an opportunity for bugs and is likely to make your code more muddled and less expressive.
    It is by no means a coincidence that performance considerations, mathematical considerations, and functionality considerations all point you to one solution.
    - Peter
    [ January 24, 2003: Message edited by: Peter den Haan ]
     
    It is sorta covered in the JavaRanch Style Guide.
     
    subject: is this lock&unlock ok?
     
    Similar Threads
    lock and unlock
    About:My URLyBird1.3.2 Locking
    Peter, please review LockManager
    bookFlight
    need lock() be synchronized?