File APIs for Java Developers
Manipulate DOC, XLS, PPT, PDF and many others from your application.
http://aspose.com/file-tools
The moose likes Developer Certification (SCJD/OCMJD) and the fly likes lock/unlock review 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 "lock/unlock review" Watch "lock/unlock review" New topic
Author

lock/unlock review

John Smith
Ranch Hand

Joined: Oct 08, 2001
Posts: 2937

I understand that you dont like my code.Could you please comment why? I know you like
the lockmanager solution better but is there any error on my code?

I am sorry, I really can't comment because I am not familiar with the underlying design. It's just that your while(true) loops and synchronized blocks don't seem natural to me.
Eugene.
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Max:
A couple of quick thoughts: you probably don't want to use an if statement in your unlock when dealing with locks.

Good point. I remember something about that from jcp but....ok so this is the new unlock:

Also, since this is the first iteration of development, I would probably just ignore the DB_LOCK issue all together for right now. Once you have the rest sorted out, that part will fit in easily.

mmmm, i dont get this more than i am not showing you the code you expcted ?
I am a little lost here. Whats next?


Free software is a matter of liberty, not price.
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
Originally posted by Raffe Paffe:
Max:


mmmm, i dont get this more than i am not showing you the code you expcted ?
I am a little lost here. Whats next?

Ok, before we move on, let's examine what you have a little more closely.
First thing, I'm pretty sure you want to synchronize outside of the while block. That is, you want to tell the JVM "don't let any other threads modify mapLocks untill I'm done". The way you're currently doing it, another thread could modify mapLocks, and your thread might never know. There's some great reading on this regarding use/read/write, thread memory, etc., in the java spec, if you're interested. But the short of it is, you can't 100% trust the state of mapLocks unless you have it in a synchronized block.
so, we're looking @

Make sense so far? I'm trying to do this in a way that doesn't steal the challange away from you, so be patient with me if I'm being somewhat circumspect.
M


Java Regular Expressions
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Hi Raffe,
This topic is good for me - I implemented my locking scheme late last week, so I get to see how someone else has done / is doing it, and I whenever anyone spots a potential problem I get to look for those same problems in my code
The following are just personal dislikes - feel free to ignore them.
I don't agree with Max about the "if" statement in the unlock code. For the unlock, you need to know that you own the lock, and if so, you need to unlock it. Even if this thread was sliced out after the if statement was evaluated as true and before the resulting actions take place, nothing will have changed - no-one else could have removed your lock. Maybe coding a while statement will be better for future enhancements (I cant see how in context), but my personal feeling is that the if statement is slightly more readable in this case.
Personal preference - I dont like having two constants which are doing the same thing - your "DATABASE_LOCK" and "ALL_DATABASE_LOCKED"
Since the only time you use the constant "DATABASE_LOCK" is in the code:

I dont think it is necessary, you could easily compare the recordNumber with ALL_DATABASE_LOCKED.
(My dislike of two constants stems from too many times debugging code where someone has had multiple constants for one purpose - this causes two problems, if the constants are defined seperately, then changing "the obvious" constant may not fix any given problem; if one is defined based on the other then changing one can side effect another).
Regards, Andrew


The Sun Certified Java Developer Exam with J2SE 5: paper version from Amazon, PDF from Apress, Online reference: Books 24x7 Personal blog
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
/*
I don't agree with Max about the "if" statement in the unlock code.

The problem here is that even as one thread could give the client the lock, another could take it away. With the while statement, you're not open to this. The point is to write threadsafe code, it's not stylistic.
M
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Hi Max,
Based on the following, I can accept that we should always use the while statement.
The point is to write threadsafe code, it's not stylistic.

However the following gives me pause...
The problem here is that even as one thread could give the client the lock, another could take it away

We are talking about the unlock code, not the lock code. I cannot see any case where any other thread could take the lock away. The test is that the lock exists and that the current thread owns it. If any other thread can unlock this threads lock then there are logic bugs that must be resolved.
If I am missing something, can you please tell me.
Thanks, Andrew
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Max:
Make sense so far? I'm trying to do this in a way that doesn't steal the challange away from you, so be patient with me if I'm being somewhat circumspect.

Yes, i will sit down tonight and think hard before i post my nesxt vesion. Thank you very much for taking the time to help me.
Andrew: I will look at the two constants tonight.
Thanks. (Code to come later)
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
Originally posted by Andrew Monkhouse:
Hi Max,
Based on the following, I can accept that we should always use the while statement.

We are talking about the unlock code, not the lock code. I cannot see any case where any other thread could take the lock away. The test is that the lock exists and that the current thread owns it. If any other thread can unlock this threads lock then there are logic bugs that must be resolved.
If I am missing something, can you please tell me.
Thanks, Andrew

Hi Andrew,
The problem, I think, is the following. You're argument rests on the assumption that a given client will lock, modify, then unlock. In this scenario, there is no room for a client to unlock a record they do not own. However, the code requirements explicitly state that a lock shouldn't be released unless the owner actually owns the lock, which makes me think that the architect who created the design has a scenario in mind which would allow this. And it follows, since this is possible, that the you need to be aware of the synchronized state of the map. Make sense?
M, author
The Sun Certified Java Developer Exam with J2SE 1.4
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Hi Max,
Ah, now I understand. Thanks for being so patient with me.
Regards, Andrew
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Hers is my unlock now:

Max, your call! :-)
Andrew i will do the "your" changes later on.
Thanks again Max!
[ April 10, 2003: Message edited by: Raffe Paffe ]
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Hi Raffe,
I think your logic for the while loop is the wrong way around. As it stands, if someone trys to unlock a record they dont have, it will block until they lock the record so that it can be unlocked ???
Perhaps:
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Andrew:
Ofcourse!! :-)
So here is the unlock again:

Ok, i will move on to my lock. I will post it here soon.
Thanks again!
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Ok, so here is my lock.

Any comments would be great.
Thanks
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Hi Raffe,
This looks good. My comments (again) have nothing to do with functionality, just with styling:
I wont repreat my comment about DATABASE_LOCK and ALL_DATABASE_LOCKED.
You have two identical pieces of code ... the catch statements after each wait. Perhaps you could change it such that:

Think about whether that makes it easier to read and / or maintain or not?
Alternatively (IMHO a better solution), since you are just catching the InterruptedException and throwing an IOException, why not get rid of the try / catch blocks altogether and just throw the InterruptedException error? Since you are not handling the exception here, the logical place for the catch is in the calling class. Is there a requirement that your calling class only handles IOExceptions?
Regards, Andrew
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Andrew:
Thanks for your feedback!
Is there a requirement that your calling class only handles IOExceptions?

I dont know, but the code provided from sun throws ioexception so i thought i will do the same. Maybe i dont have to?
Perhaps you could change it such that..

I will have a look at your change.
I have another question about this design which i asked Max but he didnt answer. What about clients that read when i write. Will it work? Max said yes, but i dont understand. If i start to write to a file and some other client reads the same record that i write will not the read be getting the wrong information? Every client has its own copy of the Data class and therfore File object. What ensures that the file will not be corrupted?
I havent change anything in Data except lock/unlock/criteriaFind and deprecated stuff. Is that enought?
Thanks again.
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
Originally posted by Raffe Paffe:
Andrew:
Thanks for your feedback!

I will have a look at your change.
I have another question about this design which i asked Max but he didnt answer. What about clients that read when i write. Will it work?
Max said yes, but i dont understand. If i start to write to a file and some other client reads the same record that i write will not the read be getting the wrong information? Every client has its own copy of the Data class and therfore File object. What ensures that the file will not be corrupted?

Hi Raffe,
Sorry I wasn't able to get to you earlier, but work's been pretty hectic. The answer to your question is no. Because of IO blocking, you're fine here. You might get 'old' data, but you're safe from 'bad' data, I think.

I havent change anything in Data except lock/unlock/criteriaFind and deprecated stuff. Is that enought?
Thanks again.

Yes.
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
Originally posted by Andrew Monkhouse:
Hi Raffe,
This looks good. My comments (again) have nothing to do with functionality, just with styling:
I wont repreat my comment about DATABASE_LOCK and ALL_DATABASE_LOCKED.

I agree with Andrew: just get rid of that stuff for right now.

You have two identical pieces of code ... the catch statements after each wait. Perhaps you could change it such that:

Think about whether that makes it easier to read and / or maintain or not?

I disagree here: leave it in, as that's more conventional.

Alternatively (IMHO a better solution), since you are just catching the InterruptedException and throwing an IOException, why not get rid of the try / catch blocks altogether and just throw the InterruptedException error? Since you are not handling the exception here, the logical place for the catch is in the calling class. Is there a requirement that your calling class only handles IOExceptions?
Regards, Andrew

Two issues here: first, tha would be changing the method signature, which mean that you're implementing a different method then the one they asked you to implement.
Second, I'm not sure you want to make your client to be aware that there's threading going on under the covers here: maybe you do, it's a judgement call, and I don't know if there's a 'right' answer: both side could be argued.
All best,
M, author
The Sun Certified Java Developer Exam with J2SE 1.4
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Max:
Because of IO blocking, you're fine here. You might get 'old' data, but you're safe from 'bad' data, I think

Ok, can yuo explain more about this or post a link? Is it up to the java implemention to handle this?
As you have not commented on my unlock/lock implemenation should i understand that you think its ok(you know what i mean)?
Thanks a lot everybody. I just want some final understanding of the IO blocking and then i will post my design for a review.
Regards
Raffe
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
Hi Raffe,
Originally posted by Raffe Paffe:
Max:

Ok, can yuo explain more about this or post a link? Is it up to the java implemention to handle this?

Not sure exactly what question you're asking here. Can you be a little more specific?


As you have not commented on my unlock/lock implemenation should i understand that you think its ok(you know what i mean)?

I though I did comment? Cleaning out the DB_LOCK stuff will help us focus on heart of the code.
All best,
M
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Max,
here is the code:

Does it look ok?
Because of IO blocking, you're fine here.

My question was about this. Can you explain more about it or post a link?
Thanks again Max.
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Hi Raffe,
Are you still implementing a LockManager class where Data.lock() calls LockManager.lock()?
When I made my suggestion that the lock() code you were showing us could throw the InterruptedException, I was still thinking in terms of a LockManager class. In which case Data.lock() could catch the InterruptedException and handle it in the method appropriate to the Data class (in this case, throw the IOException).
Both you and Max seem to think that I meant to change the Data.lock() signature - I appologise for not making it clear that this was not my intention.
Regards, Andrew
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Andrew:
Are you still implementing a LockManager class where Data.lock() calls LockManager.lock()?

No , my implemenation is in Data class. I hope your suggestions still apply. :-)
Thanks again.
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Hi Raffe,
My suggestion to allow the parent class to handle the exception no longer applies
If it is in the Data class, then you were right to be changing the thrown exception to an IOException.
Regards, Andrew
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Thanks for all your help Andrew!!
Max, do you think that lock() is good enought or can more be done? If yes, what?
Oftopic: I have run checkstyle with ant on the supplied classes from sun and get a lot of warnings. Should i change the style in them or just leave them?
Thanks a lot Max.
[ April 13, 2003: Message edited by: Raffe Paffe ]
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
It looks Fine Raffe: now write a test client that spawns a 100 threads, and test the hell out of it .
As for the style: sure, I would review the suggestions, and see which ones made sense.
M
Raffe Paffe
Ranch Hand

Joined: Feb 24, 2003
Posts: 92
Max:
Thanks for all your help! Its been great!
Now, can you provide some more info on IO Blocking?
I am not clear on that one.
Thanks
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
In general, when a single thread is working with IO, that that thread is blocked for the duration of the IO call. That is, when a given GUI client actually get to point in your code when you're executing a method on the File object itself(or FileChannnel, or RandomAccessFile, etc.,), that GUI client will be 'frozen' until the method is done executing. Does that make sense?
M, author
The Sun Certified Java Developer Exam with J2SE 1.4
Matthew Anderson
Ranch Hand

Joined: Apr 18, 2003
Posts: 33
Hi Max
Based on Raffe Paffe's lock and unlock code, I've found some of the differences between his code and the code that you have provided in your book.
Raffe uses a static map which stores the record number as the key and the entire Data as the value. In your code, you use a Vector to store the record number instead. Which data structure is a better approach? Is it necessary to store an entire Data object into the map?
Another difference is that Raffe's unlock method checks for the Data object's existence in the map before removing. But you have stated in the book that:
"We simply achieve a lock on the reservedDVDs vector and remove the upc from it. We don't really have anything to check, so it doesn't even matter if the UPC question is not locked."
Shouldn't that applies to the current context as well?
Thanks.


SCJP 1.2, SCWCD, SCJD
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
To be honest, I was worried about giving too much away while writing the book: I still worry about it. That would be a disservice not only to Sun, but to all the excellent developers out there are working hard to create their own solutions. The code is the book is not meant to be an answer to Sun's SCJD, just to the DVD store sample project. As such, the general principle used can be applied to the SCJD, but not the specific answers given.
All best,
M, author
The Sun Certified Java Developer Exam with J2SE 1.4
John Smith
Ranch Hand

Joined: Oct 08, 2001
Posts: 2937
Raffe uses a static map which stores the record number as the key and the entire Data as the value. In your code, you use a Vector to store the record number instead. Which data structure is a better approach?
Vector is a legacy collection, -- it should never be used. You have an ArrayList to achive the same purpose, and you can make it synchronized if you want to. For the SCJD assignment, the most appropriate collection to store the record locks is the hash map, -- I think everybody agrees with that.
Eugene.
Rajesh So
Ranch Hand

Joined: Oct 08, 2002
Posts: 124

This would remove the word 'synchronized' from the entire Data class.
Am I right ?
[ April 21, 2003: Message edited by: Rajesh Rajesh ]
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
Originally posted by Eugene Kononov:
[b]
Vector is a legacy collection, -- it should never be used. You have an ArrayList to achive the same purpose, and you can make it synchronized if you want to. For the SCJD assignment, the most appropriate collection to store the record locks is the hash map, -- I think everybody agrees with that.
Eugene.

This sort of blanket statement is misleading. Vectors have their place, as do Arraylists. That's why the wise folks @ Sun didn't depreciate Vectors, as they did Enumerations. However, yes, you want to use a Map structure here.
M
Max Habibi
town drunk
( and author)
Sheriff

Joined: Jun 27, 2002
Posts: 4118
Originally posted by Rajesh Rajesh:

This would remove the word 'synchronized' from the entire Data class.
Am I right ?
[ April 21, 2003: Message edited by: Rajesh Rajesh ]

Afraid not Rajesh. This only means that calling a method on the map is an atomic activity, not that method executing those calls are. I dig into this quite a bit in the book, because it's a common, and reasonable, misunderstanding.
M
Rajesh So
Ranch Hand

Joined: Oct 08, 2002
Posts: 124
Max,
I have removed as you suggested. Also, it was not possible to notify, if there was a synchronized map (blocks give flexibility).
When I tried to add records on the db with more than one client, I got the following error.

Eugene expressed the concern regarding the file pointer. The above problem could be because of that.
For example, if one thread wants to modify, it would first search using seek()->getRecord()-> ->modify(). When this thread has placed the pointer through seek(), the second thread which wants to read, seek()-> readRecord() could place the pointer in some other place. This would clash with the modify operation. Please tell me Max if I am wrong.
Secondly, if the company plans to use one more database file in future, they would have to use one more static Map in the Data class. Is it okay to have many static Maps?
Rajesh
[ April 24, 2003: Message edited by: Rajesh Rajesh ]
Rajesh So
Ranch Hand

Joined: Oct 08, 2002
Posts: 124
Hello Max,
I posted the above after testing for add() only. But your idea works perfectly fine for delete(), modify(), lock(), unlock() and all methods of Data (except add()) for 200+ multi threads. There was no errors.
The add() operation error is not because of Max's principle. I tried to add() from several threads and above error popped up. Although add() is not for assignement, its lack of support for multi threads persists. Will not solving add() reduce marks???
If the question in essay exam asks how do you plan to upgrade Data for more than one database, the answer would be to have a static Map for every Database. Is this practice of having a number of static maps a good practice?
Rajesh
John Smith
Ranch Hand

Joined: Oct 08, 2001
Posts: 2937

If the question in essay exam asks how do you plan to upgrade Data for more than one database, the answer would be to have a static Map for every Database.

So, if you have 20 database files, you would have 20 static maps?
Qusay Jaafar
Ranch Hand

Joined: May 06, 2002
Posts: 127
What is the problem if each HashMap control a database???
Regards


Qusay
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

I think the answer to that would be getting well outside of the scope of this assignement.
However the basic answer is the amount of space required for all these locks.
If you have 20 tables to lock, each with 100 records, you will be allocating space for 2,000 key / value pairs plus any class overhead.
If the tables themselves are only 5% utilized at any given time, then that is 1,900 unused key / value pairs unused.
If you are working in a limited memory model, then this could be a lot of wasted space.
So many ifs .... that is why I say this is outside scope
But there could be other ways of handling this (such as making the key a combination of a unique identifier for the database along with the record identifier - and possibly overriding the hashValue and equals methods) which could reduce your memory requirements.
But before you even think of looking at this (YAGNI principle) you would need to be in a situation where you are locking multiple tables, you do have limited available memory, and your table locking requirements are such that optimizations can be made.
As I keep saying ... way outside of scope.
Regards, Andrew
Andrew Monkhouse
author and jackaroo
Marshal Commander

Joined: Mar 28, 2003
Posts: 11280
    
  59

Oh, and you would also have to think about how you associate the 20 static maps with the relative databases.
Qusay Jaafar
Ranch Hand

Joined: May 06, 2002
Posts: 127
And what about future enhancements that assignment instruction states about it. I read some threads here about this and about reducing marks because of not dealing with multiple Databases???
And I think the argument between Max and Eugen implicitly about that too???
Regards
 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: lock/unlock review
 
Similar Threads
About:My URLyBird1.3.2 Locking
Peter, please review LockManager
lock/unlock
why is it necessary for unLock() to check clientID?
Locking/Unlocking, -- Am I Done Or I Don't Understand Requirements?