NX: Are my Lock / Unlock methods OK?

[Jacques Bosch]

Hi all you boffins.
Do my Lock / Unlock methods look OK?
lockedRecNumbers is a HashMap.

    public long lockRecord(long recNo) throws RecordNotFoundException {
    long newLockCookie = -1;
synchronized (lockedRecNumbers) {
            /*
             * If this record number is already locked, wait for the lock
             * to be removed, and then lock it with the new cookie.
             */
            while (lockedRecNumbers.containsKey(new Long(recNo))) {
                //log.info("Waiting = " + Thread.currentThread().getName());
                try {
                lockedRecNumbers.wait();
                } catch (InterruptedException e) {
                // What should I do here?
                }
            }
    
        newLockCookie = System.currentTimeMillis();
        lockedRecNumbers.put(new Long(recNo), new Long(newLockCookie));
        lockedRecNumbers.notifyAll();
}
    return newLockCookie;
    }

Thanx much!
[ January 14, 2004: Message edited by: Andrew Monkhouse ]

[Terry Martinson]

I throw a runtime LockException (defined by me) in the case of that InterruptedException you catch in the lock.
Also, I have seen in this forum comments regarding the notifyAll call being useless in the lock. So, I only have the notifyAll in the unlock.
I don't know offhand what posting this was, but you could search on "notifyAll". I think Phil may have been a part of that discussion, so you could include his ranch number in your search if needed.
TJ

[John Smith]

To second Terry, there is no need to notifyAll() after the record is locked, -- no one is waiting for it. All it would do is to awaken all the threads that are waiting for some other records to be unlocked, only to realize that the wake up call was an unneeded disturbance to the CPU.
In addition, I am interested to hear your motivations for the current system time as a choice for a cookie. Are you sure that your cookies are guaranteed to be unique between the clients? It also looks like that the cookie will change all the time for a given client, which may be a problem is debugging/diagnosing the potential problems.

[Jacques Bosch]

Hi Terry.

I throw a runtime LockException (defined by me) in the case of that InterruptedException you catch in the lock.

And then you catch that where and do what with it? 8)
Eugene,

In addition, I am interested to hear your motivations for the current system time as a choice for a cookie. Are you sure that your cookies are guaranteed to be unique between the clients? It also looks like that the cookie will change all the time for a given client, which may be a problem is debugging/diagnosing the potential problems.

You can check out here: https://coderanch.com/t/184895/java-developer-SCJD/certification/NX-lockCookie-generation

[Derek Canaan]

Hi Jacques,

[Terry]:I think Phil may have been a part of that discussion ..

- Phil, Vlad, Jim and Max slogged it out in thread 5641. In the previous FBN assignment, there are some possible designs that need a notifyAll for lock().
In another post (thread 5698), Andrew mentioned that whenever a thread exits the sync block, the JVM will automatically tell other waiting threads that the mutex is available, so there is no need for notifyAll whichexplicitly does the notification.
rgds,
derek

[Jacques Bosch]

OK, I've read the unbelievably long thread 5641......!
Now my question is:
So notifyAll() is not needed in the lock method, but it sounds to me like not even notify() is required.
Am I missing something. (Obviously )

[Terry Martinson]

I believe what you say is true.
In the lock, we don't need either notify() or notifyAll().
In the unlock I use notifyAll() (like Max book)
TJ

[Jacques Bosch]

Thanx Terry .
Anybody else have a comment?

[Jim Yingst]

Correct, neither notify() nor notifyAll() is needed after a lock.

[Manoj Gundawar]

I guess you might need to handle the following situation too. (This depends on implementation of your other methods and also depends on how you are calling lock and unlock method)
1. Thread 1 is having a lock for record 10.
2. Thread 2 is waiting for Lock of record 10.
3. After getting loc, thread 1 actually perform delete operation on record 10 and then releases the lock.
4. Now do you still want thread 2 to get the lock of the record which does not exist at all? what you do after that?
So I guess if after wait is over, and just before locking the record, you might need to check if the record is valid or not.
Just a thought.
Manoj

[Andrew Monkhouse]

Hi Jacques,
As you can see, I have removed the unlock() method from your post.
While we do allow code to be posted in this forum, we try to limit the amount of code posted. Having both the lock() and unlock() methods posted meant that anyone reading your post could immediately get 20% of the assignment. At least with only one method, they will still have to do some work to build the other half.
According to your method signature, the lock() method can throw RecordNotFoundException, however you do not appear to be throwing this exception. Have you considered when you might have to throw it?
Regards, Andrew

[David Chan]

I don't understand why neither notify() nor notifyAll() is needed in this lock method. If remove the notify method, then all other threads have to wait because lockedRecNumbers (I think this object is declared as static) is being locked.
lock
update (maybe take long time to process)
unlock
Can some explain this? Thank you.

David

[Jacques Bosch]

Jmannu:

Start of lock method (...) {
   synchronized (raf) {
      Check if rec number is valid / exists.
   }
   synchronized (lockedRecNumbersHashMap) {
      Try to get the lock as shown in my post at the top.
   }
} End of lock method

I do all my synchronization like this throughout my Data class. I.e. I/O operations are done in synchronized blocks on the raf, and locking / locking checks are synchronized on the locking HashMap. These always follow one after the other in any one method. They are never synchronized at the same time in the same method. Not nested.
Now my question is, will the way I did this be OK, or might it cause problems?
I think it's fine, because locking doesn't really have any thing to do with I/O directly.
Comments?
[ January 14, 2004: Message edited by: Jacques Bosch ]

[Jacques Bosch]

David,

I don't understand why neither notify() nor notifyAll() is needed in this lock method. If remove the notify method, then all other threads have to wait because lockedRecNumbers (I think this object is declared as static) is being locked.

Only the lock method ever waits inside the synch block. The unlock method never waits. Therefore only the unlock has to notify when a rec has been unlocked. To let lock method know to try again.
[ January 14, 2004: Message edited by: Jacques Bosch ]

[Jacques Bosch]

OK, my question was:

I do all my synchronization like this throughout my Data class. I.e. I/O operations are done in synchronized blocks on the raf, and locking / locking checks are synchronized on the locking HashMap. These always follow one after the other in any one method. They are never synchronized at the same time in the same method. Not nested.
Now my question is, will the way I did this be OK, or might it cause problems?
I think it's fine, because locking doesn't really have any thing to do with I/O directly.

But now, after Jmannu's comment:

So I guess if after wait is over, and just before locking the record, you might need to check if the record is valid or not.

I have to do a record validation check just before locking a rec, in a nested synch block, like below. Is this OK?

Start of lock method (...) {
   synchronized (raf) {
      Check if rec number is valid / exists.
   }
    synchronized (lockedRecNumbersHashMap) {
        Wait for rec to be unlocked.
        synchronized (raf) {
            Check again if rec number is valid / exists (Was maybe deleted).
        }
        lock the record if it was valid and unlocked.
    }
} End of lock method

[ January 14, 2004: Message edited by: Jacques Bosch ]

[Manoj Gundawar]

Yes. That's what I meant. Otherwise it will end up locking a deleted record. But you can avoid this code too if you properly use the unlock method.
For ex, without adding this check in the lock method, if your operations are done in the following sequence, it will not cause problems anymore.
1. Current thread Locks the record which was deleted by earlier thread.
2. Current thread then comes to update method (or some method after lock which tries to modify/delete record) And finds that record does not exist and hence throws the RNF exception. (rec not found)
3. Current thread still unlocks this record, even if the update/delete operation fails.
With this logic, even the deleted record number gets locked, it wont hurt. But it is good to not lock it at the first place itself.
Think about it.
Manoj

[Jacques Bosch]

That's exactly how I specified the methods to be used.
So then I don't really need the second check.
And about my synch question?
J
[ January 15, 2004: Message edited by: Jacques Bosch ]

[Jacques Bosch]

Even for unlocking, notify() should suffice in stead of notifyAll() since only one other thread/client will be able to get the lock. So, you're actually slowing the system if you wake up all the threads.
Missing something again?
Using notify() only:
I started up 30 threads that all tried to loop through the same 3 recs and modify them, and it works perfectly every time.
[ January 15, 2004: Message edited by: Jacques Bosch ]
I moved this question to: https://coderanch.com/t/184917/java-developer-SCJD/certification/Threads
[ January 15, 2004: Message edited by: Jacques Bosch ]

[Peter Yunguang Qiu]

Hi,
How to fine thread 5641, 5698?

[Andrew Monkhouse]

Hi Peter,

How to fine thread 5641, 5698?

If you take a look in your browser's address field, you should see something like:
https://coderanch.com/t/184896/java-developer-SCJD/certification/NX-Lock-Unlock-methods-OK
The last six digits are the topic number. So to go to topic 5641 you would enter the URL
https://coderanch.com/t/184174/java-developer-SCJD/certification/NX-URLYBird-approach-reading
And to go to topic 5698 you would enter the URL
https://coderanch.com/t/184230/java-developer-SCJD/certification/lock-update-unlock
You usually dont have to do this - most people put links to the relevant topics which you can click on: 5641 & 5698.
Regards, Andrew