• Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

NX: Breaking up Unlock to local methods

 
Bill Robertson
Ranch Hand
Posts: 234
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:

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


OR
 
Vlad Rabkin
Ranch Hand
Posts: 555
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 234
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 234
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Help please. This is driving me over the cliff.
 
Andrew Monkhouse
author and jackaroo
Marshal Commander
Pie
Posts: 11887
203
C++ Firefox Browser IntelliJ IDE Java Mac Oracle
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
    Ranch Hand
    Posts: 234
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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:


    and thats it.
     
    Bill Robertson
    Ranch Hand
    Posts: 234
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 555
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    author and jackaroo
    Marshal Commander
    Pie
    Posts: 11887
    203
    C++ Firefox Browser IntelliJ IDE Java Mac Oracle
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 234
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thanks guys, I have made my decision.
     
    • Post Reply
    • Bookmark Topic Watch Topic
    • New Topic