NX: Breaking up Unlock to local methods

[Bill Robertson]

In some older threads Andrew and a few others mentioned breaking up
unlock. Sounds like a great idea. But I have a few questions regarding this.
My break down would look like:

 
public void unlock(int recordNumber, long cookie)
            throws RecordNotFoundException, SecurityException {
   synchronized(this.lockedCntrRecords) {
      unlockValidateRecordExists(recordNumber)
      unlockValidateSecurity(recordNumber,cookie)
      unlockRemoveFromCollection(recordNumber)
   }
}

If I synchronize on my collection lockedCntrRecords, should I synchronize on that collection within each of the methods? Or would it just be easier to synchronize the different methods? Or is it just a matter of opinion?
Example

  
public void unlockRemoveFromCollection(int recordNumber) {
   synchronized(this.lockedCntrRecords) {
      this.lockedCntrRecords.remove(String.valueOf(recordNumber));
      this.lockedCntrRecords.notifyAll();
   }
}

OR

public synchronized void unlockRemoveFromCollection(int recordNumber) {
   this.lockedCntrRecords.remove(String.valueOf(recordNumber));
   this.lockedCntrRecords.notifyAll();
}

[Vlad Rabkin]

Hi Bill,

If I synchronize on my collection lockedCntrRecords, should I synchronize on that collection within each of the methods?

No way!
You first sample of the unlock() looks great is valid.
You have to guarantee atomicity of the unlock() transaction.
If you synchronizie each submethod separately no atomicity of the unlock() will be guaranteed.
Best,
Vlad

[Bill Robertson]

Thanks Vlad, but this raises some questions.

No way!
You first sample of the unlock() looks great is valid.
You have to guarantee atomicity of the unlock() transaction.
If you synchronizie each submethod separately no atomicity of the unlock() will be guaranteed.

So in other words forget any synchronization on the other three methods.
OK, but can you explain to me how that breaks atomicity rather than keeps it? And remember I will be calling unlockRemoveFromCollection(int recordNumber) from my delete method or any method that throws RecordNotFoundException (Which is the point of breaking the unlock method up in the first place). Must I not then own the lock on
lockedCntrRecords in the method to call notifyAll()?
Am I lost?

[Bill Robertson]

Help please. This is driving me over the cliff.

[Andrew Monkhouse]

Hi Bill,
Surprisingly I disagree with Vlad on this one: I don't think you need atomicity for those three operations as a whole. Taking each of them in turn:

unlockValidateRecordExists(recordNumber)
If, after validating the record exists, you loose the mutex on lockedCntrRecords, and someone deletes the record, so what? The current thread will just continue as though the record was valid, and it will then fail on the next test (since for someone else to delete the record, they must own the lock). So the results will be the same as if you had guaranteed atomicity around the three method calls.

unlockValidateSecurity(recordNumber,cookie)
Again this should not cause us a problem. If you loose the mutex on lockedCntrRecords after this call, you will still own the lock on the record so when you regain the mutex nothing will have changed.

unlockRemoveFromCollection(recordNumber)
Nothing to say.

As for moving the synchronization into those methods: As you have noted, you will need to own the mutex on lockedCntrRecords in order to call notifyAll(), so this is a good thing.
Regards, Andrew

[Bill Robertson]

Thanks Andrew, I was hoping you would reply.
I have a question though. The main reason I am doing this is so I can
call unlockRemoveFromCollection(recordNumber) from other methods
before they throw an exception. Would it be cleaner to just leave
my original unlock as is and create another unlock method that takes just the record number and looks something like:

 public void lock(int recordNumber) {
     synchronized(this.lockedCntrRecords) {      
         this.lockedCntrRecords.remove(String.valueOf(recordNumber)); 
  
         this.lockedCntrRecords.notifyAll();   
     }
 }

and thats it.

[Bill Robertson]

somehow I posted by accident. I wanted to add that even if
this.lockedCntrRecords.remove(String.valueOf(recordNumber)) returns false
so what. But maybe then I should not call notifyAll.

[Vlad Rabkin]

Hi Bill and Andrew,
Andrew:

If, after validating the record exists, you loose the mutex on lockedCntrRecords, and someone deletes the record, so what?

Ups. I agree. Honestly saying I thought about lock() method. It is much more tricky there. Regarding unlock() I agree with Andrew and recognize my mistake in my previous mail.
Sorry.
Best,
Vlad

[Andrew Monkhouse]

Hi Bill,
I think it is better to only have one method which removes the record number from the collection - having it in two places invites problems. And I would only call notifyAll() if you actually removed a record from the collection.
Regards, Andrew

[Bill Robertson]

Thanks guys, I have made my decision.