aspose file tools*
The moose likes Developer Certification (SCJD/OCMJD) and the fly likes synchronized lock/unlock -> deadlock? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Certification » Developer Certification (SCJD/OCMJD)
Bookmark "synchronized lock/unlock -> deadlock?" Watch "synchronized lock/unlock -> deadlock?" New topic
Author

synchronized lock/unlock -> deadlock?

Amund Frislie
Greenhorn

Joined: Jan 25, 2002
Posts: 17
Hi.
I've implemented my lock/unlock methods according to "the book", or at least, that's what I thought untill starting the testing... Here's what I have (excluding some checking of valid record number):

It fails the following testing: I create 2 clients, accessing the same dataserver (using RMI). Client 1 locks a record (works fine). Client 2 tries to lock the same record and ends up at lockMap.wait(). Client 1 tries to unlock the record and it "crashes". I can see client 1 calling the lock method in the class which connects to the server, but there it stops (i.e. I can not see control is transferred to the server). All these steps are done manually (i.e. I handle all locking/unlocking explicitly). I use a LockManager (singleton) to handle the locking. The LockManager is accessed via the Data class (should this also be a Singleton?). Appreciate any inputs on this. Thanks.
Amund
p.s: I decided to start a new thread on this, although it is a continuation of what's found at:
http://www.coderanch.com/t/180191/java-developer-SCJD/certification/record-lock-unlock - this is getting rather long, though.
[ April 16, 2002: Message edited by: Amund Frislie ]
Sai Prasad
Ranch Hand

Joined: Feb 25, 2002
Posts: 560
Amund,
I know how you feel. I assume that the code you posted belongs to LockManager class. I am not sure why the server didn't receive the call from the client. Anyway you can try couple of things:
1) Try HashTable instead of HashMap since the methods in HashTable are synchronized
2) I know some people are against this idea. I would move the wait() and notifyAll() to the Data instance and check for the clientId in the LockManager class. In this design, you have to now synchronize the LockManager instance and leave the access to Data non-synchronized.
Also, you could use the remote implementation object as the key to the HashTable instead of having a seperate clientId.
Once you are comfortable with the design in 2), you can move back to the original design and debug more...
[ April 16, 2002: Message edited by: Sai Prasad ]
Mark Spritzler
ranger
Sheriff

Joined: Feb 05, 2001
Posts: 17259
    
    6

1. Don't use a HashTable. It is from the older collections objects. The newer collection objects like HashSet, ArrayList is much better and more preferred.
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
Heh.
  • Don't replace the HashMap by a Hashtable: (a) as Mark says, it's obsolete (b) it's got this horrid API because it was retrofitted into the Collections framework (c) it won't make a difference except add more synchronization overhead (d) it might lure you into writing thread-unsafe code.
  • Given that the relationship between LockManager and Map is 1:1 anyway, consider using simple synchronized methods rather than the laborious synchronization on lockMap -- really you're adding all kinds of clutter for no reason at all.
  • Arguably your lock() method has a bug, it really should ignore attempts to lock the same record twice.
  • Don't make either LockManager or Data a Singleton even though for FBN you need but one of each. You are coding for reuse, and I have yet to see a database with just one table in it. There is no fundamental reason why there can be only one Data (and therefore only one LockManager), so they should not be a Singleton, full stop.
  • A speculative one: your problem might be that you're calling LockManager from within synchronized methods in Data, and your lockMap.wait() only releases the monitor lock held on lockMap, not the one on Data. You must either call LockManager from a non-synchronized Data method, or synchronize everything on Data.
  • HTH,
    - Peter
    Rene Larsen
    Ranch Hand

    Joined: Oct 12, 2001
    Posts: 1179

    Hi
    I think you have a bug in your unlock method, where you are validating the userId

    You are making a check if recordInt (an Integer) is the same as userId (a String)...
    Try to change the code to:

    Hope this helps
    Ren´┐Ż


    Regards, Rene Larsen
    Dropbox Invite
    Amund Frislie
    Greenhorn

    Joined: Jan 25, 2002
    Posts: 17
    Thank you all.

    Sai: Also, you could use the remote implementation object as the key to the HashTable instead of having a seperate clientId.

    Do you mean using Thread.currentThread().getName()? I tried this, and it works. The function returns a string of this format: "RMI TCP Connection(9)-10.10.53.80", where the number within the brackets (9) differs if there are multiple clients on the same computer. Could I rely on this being unique every time? It would save me generating userId's.


    Peter: Arguably your lock() method has a bug, it really should ignore attempts to lock the same record twice.

    Why? If a client tries to lock a record that's already in the the map, it means that it's "taken" and the client should wait untill it's released. I do however check whether the same client already has a lock on the record, but omitted it for clearity...

    your problem might be that you're calling LockManager from within synchronized methods in Data, and your lockMap.wait() only releases the monitor lock held on lockMap, not the one on Data. You must either call LockManager from a non-synchronized Data method, or synchronize everything on Data.

    Hooray, found it! I had ehem... for unknown reasons synchronized the lock and unlock methods in the server class.

    Rene: the lockMap.get(recordInt) returns the value (a string in this case) pointed to by the key, not the key.
    Appreciate all your contributions. Well, back to testing. Thanks.
    Cheers,
    Amund
    Sai Prasad
    Ranch Hand

    Joined: Feb 25, 2002
    Posts: 560
    Do you mean using Thread.currentThread().getName()? I tried this, and it works. The function returns a string of this format: "RMI TCP Connection(9)-10.10.53.80", where the number within the brackets (9) differs if there are multiple clients on the same computer. Could I rely on this being unique every time? It would save me generating userId's.
    No. Do not rely on this name to be unique. It won't be. It depends on where you have the LockManager. If you have the LockManager as pass through class between the Remote implementation of Data interface and the Data class, you could use the key as the Remote implementation object itself.
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Originally posted by Amund Frislie:
    Do you mean using Thread.currentThread().getName()? I tried this, and it works.
    Seconding SP -- it doesn't work. At all. RMI does not give any guarantees about how or if threads are associated with callers.
    Why [does the lock() method have a bug]? [...] I do however check whether the same client already has a lock on the record, but omitted it for clearity...
    Alright, in that case forget what I said, erm, wrote.
    Hooray, found it! I had ehem... for unknown reasons synchronized the lock and unlock methods in the server class.
    Thought you might
    - Peter
     
    I agree. Here's the link: http://aspose.com/file-tools
     
    subject: synchronized lock/unlock -> deadlock?