NX: URLy Bird 1.3.1 Explicit Fatal Exception Handling

Hello Andrew/Max, I am picking up where we left off on this thread on custom exception handling that we were discussing in the following thread:NX: About data consistent Max, you wrote:

Just as an FYI, I would argue that the InteruptedException should cause an explicit fatal exception to thrown: one that ripples up to middle tier as a FatalSystemException, and eventually to the user as a FatalGUIException, and which cause the system to shutdown. This is how most apps deal with this sort of thing in the real world. M

Andrew, here is your quote:

Slightly more drastic than the assert. You are saying that the event should not have occured, and since it has occurred you do not believe your system should continue running at all. If you want to do this, I would recommend you subclass RuntimeException, so you are throwing an exception that has a descriptive name so that any programmer can see what you are doing.

Here is my lock method in the Data class:

As you can see above, I have tried to follow through your advice on creating a custom exception that extends the RuntimeException as follows:

I keep it in a separate package (suncertify.exceptions) since I think that this exception can be thrown both from the database tier and the middle tier. Now since FatalSystemException is extending the RuntimeException, it doesn't need to be declared. I am trying to chain this exception all the way up to the GUI where the ApplicationRunner's HandleException method can display a meaningful message to the user. Here is what the JavaDoc says about this:

A second reason that a throwable may have a cause is that the method that throws it must conform to a general-purpose interface that does not permit the method to throw the cause directly. For example, suppose a persistent collection conforms to the Collection interface, and that its persistence is implemented atop java.io. Suppose the internals of the put method can throw an IOException. The implementation can communicate the details of the IOException to its caller while conforming to the Collection interface by wrapping the IOException in an appropriate unchecked exception. (The specification for the persistent collection should indicate that it is capable of throwing such exceptions.) A cause can be associated with a throwable in two ways: via a constructor that takes the cause as an argument, or via the initCause(Throwable) method. New throwable classes that wish to allow causes to be associated with them should provide constructors that take a cause and delegate (perhaps indirectly) to one of the Throwable constructors that takes a cause. For example: try { lowLevelOp(); } catch (LowLevelException le) { throw new HighLevelException(le); // Chaining-aware constructor } Because the initCause method is public, it allows a cause to be associated with any throwable, even a "legacy throwable" whose implementation predates the addition of the exception chaining mechanism to Throwable. For example: try { lowLevelOp(); } catch (LowLevelException le) { throw (HighLevelException) new HighLevelException().initCause(le); // Legacy constructor }

Therefore, the book method of DataAdapter class above can throw the FatalSystemException which should be caught. Here is where I get on uncertain grounds: I think I should catch it as follows:

I will follow through the network layer later. My questions for now are: 1. Is this correct way of exception chaing for these kinds of exception. Another exception that comes to my mind is the Magic Cookie value mismatch situation. Is that correct? 2. Should I be rethrowing the FatalSystemException as I show above? I was thinking the message string does not really add any value. In that case, should I catch the FatalSystemException and simply rethrow it all the way up until I can wrap it into GUIControllerException which can then be displayed by the ApplicationRunner class's handleException method? I dare not write anymore, this will turn into a dissertation! I will await your comments. Thanks. Bharat

Hmmm - your code doesn't quite match up here. In your Data.lock() method, when you catch the InterruptedException, you are using the default constructor of FatalSystemException - you are not setting the cause. Yet later you try and get the cause from the thrown Exception (by the way - in your book() method, where you catch the FatalSystemException, you should check to see if whether you were able to retrieve the cause.

[talking about exception chaining] Another exception that comes to my mind is the Magic Cookie value mismatch situation.

Personally I don't think so. I believe that on application startup (both stand alone client and server) you should verify the cookie. Even if you do nothing else and then close the application. From then onwards you should be safe with cookie - the instructions tell you that no other application will modify the file while your application owns it. So you could probably have the check either in the Data constructor (in which case you can throw any exception you like) or in your own method in the Data class (again you can throw any exception you like). You should not need to chain exceptions in this case. (As an aside: You do (IMHO) want this check on application start up. There would be nothing worse than starting the server application, going home before anyone tries to do a booking, and later that the server crashed the first time someone tried to do the booking all because you had an invalid data file and it wasnt checked at startup. Much better to do a simple check at application startup and shut the server down instantly if the file is wrong.)

Should I be rethrowing the FatalSystemException as I show above?

Hmmmm. You are actually wrapping the FatalSystemException inside another FatalSystemException. Is there any reason for this? I can see value in both catching and rethrowing the FatalSystemException. Catching it lets the server deal with it (logging it, and/or shutting down). While rethrowing it lets it propogate up to the client, so that someone is aware of what the real problem is. The client can then scream to the sys admin that the server crashed. By the way - I think your RecordNotFoundException in the book() method should probably be propogated in some way back to the client. And I am not sure about throwing a RecordNotFoundException if the client already owns a lock.

I think you have to avoid coupling the GUI to the DB via custom Exceptions. Tony

Bharat Ruparel

Ranch Hand

Posts: 493

posted 13 years ago

Hello Andrew, First of all, thanks for getting back to me so quickly. You wrote:

Hmmm - your code doesn't quite match up here. In your Data.lock() method, when you catch the InterruptedException, you are using the default constructor of FatalSystemException - you are not setting the cause. Yet later you try and get the cause from the thrown Exception (by the way - in your book() method, where you catch the FatalSystemException, you should check to see if whether you were able to retrieve the cause.

My apologies for misleading you, actually I was programming and writing this post at the same time - not recommended. I did discover the point that you are making above and ended up correcting my code but not the post. Here is the modified code:

You can see that I am not using the default constructor and passing the cause to the FatalSystemException. Is this OK? I will come back to the Magic Cookie topic later. I do have some follow-up questions. I want to get exception-chaining right first. Consider the following code:

You wrote:

Hmmmm. You are actually wrapping the FatalSystemException inside another FatalSystemException. Is there any reason for this?

I was trying to update the String "msg" as it passes up the exception chain by doing the following: msg += "Bad threading error in the lock method!" I can change the message to something more appropriate, but you get the idea. Another correction in the code that I posted first is that I should have written: throw new FatalSystemException(msg, fse) instead of throw new FatalSystemException(fse) Sorry. You wrote:

I can see value in both catching and rethrowing the FatalSystemException. Catching it lets the server deal with it (logging it, and/or shutting down). While rethrowing it lets it propogate up to the client, so that someone is aware of what the real problem is. The client can then scream to the sys admin that the server crashed.

Thanks for validating my thinking. I will add appropriate logging code - I haven't added any logging code yet. Things seemed to have work suprisingly well! Few sessions with the debugger is all that I have needed. I will pursue the "added value" of logging with you and Max in a separate thread. You wrote:

By the way - I think your RecordNotFoundException in the book() method should probably be propogated in some way back to the client.

I agreee, I haven't yet worked out how should I do that. My initial thoughts: 1. Should I rethrow it with an appropriate added message string as I do above? 2. Should I throw a custom exception up the chain? - if yes then can you give me an example? You wrote:

And I am not sure about throwing a RecordNotFoundException if the client already owns a lock.

The reason that I did that is my Data class contains the lock method which declares that it throws the RecordNotFoundException. The lock method first validates the record by calling validateRecord(recNo) private method which will throw RecordNotFoundException if it is an invalid record. This will happen before the client acquires a lock. By the way, is this another example of FatalSystemException type exception handling (I don't think so), or should it be wrapped into a new custom exception which is appropriate for the "DataAdapter" layer? I am not sure.. I shall look forward to your response. Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Bharat Ruparel

Ranch Hand

Posts: 493

posted 13 years ago

Hello Tony, Thanks for your response. I agree with you that we should not "tightly" couple the GUI and the DB layer, but there has to be a "loose" coupling, otherwise the client will have no idea about "stuff" happening deep down within. Now, one should not pass-on the gory details to a "naive" client, but a message wrapped in somewhat user-friendly language (I know it is a bit of a contradiction; conveying bad news in a good way), but that is what we are paid for I guess. Regards. Bharat

The chaining facility was designed to give more information to those clients that want to look at the cause. The way you are doing it forces someone to look at the cause. This is legal, but personally I think it would be nicer if you gave a descriptive message to go with it. So ...

[Andrew] By the way - I think your RecordNotFoundException in the book() method should probably be propogated in some way back to the client. [Bharat] I agreee, I haven't yet worked out how should I do that. My initial thoughts: 1. Should I rethrow it with an appropriate added message string as I do above? 2. Should I throw a custom exception up the chain? - if yes then can you give me an example?

Personally I wouldn't even bother catching it. Just declare that your book() method throws the RecordNotFoundException, so if it occurs in your try block, it will automatically get propogated to the client. I don't think you need to change the message received from your Data class, or chain it. The RecordNotFoundException is probably exactly what your client needs to receive. [If we really wanted to do some defensive programming, you could probably subclass RecordNotFoundException so that you have exceptions for record number too low, record number too high, or deleted records, then you could catch them in your book method, log them, then throw them up to the client. That way you could analyise the log later and see if a particular client is continually trying something silly. But this is certainly not needed for this particular project, so let's not go there ]

[Andrew] And I am not sure about throwing a RecordNotFoundException if the client already owns a lock. [Bharat] By the way, is this another example of FatalSystemException type exception handling (I don't think so)

I think we could argue that it is a FatalSystemException. After all, the clients we are developing should only ever try to gain one lock. If they try to gain more locks, then there are a whole bunch of issues that we have not catered for (and are not required in the specifications). So if they do try to get more than one lock, then they are going to cause the system to go into a state that has not been coded / tested. However this may be a case where you want to catch the FatalSystemException in your book() method, determine that it occured because of attempting to lock more than one record, and just log it server side (don't shut server down) and then pass it up to the client. The server trapped it before it got into the invalid state, so you should be safe to carry on. Regards, Andrew

The chaining facility was designed to give more information to those clients that want to look at the cause. The way you are doing it forces someone to look at the cause. This is legal, but personally I think it would be nicer if you gave a descriptive message to go with it. So ... throw new FatalSystemException("Invalid state", e); //or throw new FatalSystemException(e.getMessage(), e);

Got it and corrected. Your quote:

Personally I wouldn't even bother catching it. Just declare that your book() method throws the RecordNotFoundException, so if it occurs in your try block, it will automatically get propogated to the client. I don't think you need to change the message received from your Data class, or chain it. The RecordNotFoundException is probably exactly what your client needs to receive.

Another good point! A reminder, I am using RMI. I have a DataImpl class wrapping the DataAdapter class as follows:

As you can see above, I have also declared RecordNotFoundException in the book method for DataRemoteImpl class. By the way DataRemote interface extends both DBClient and Remote interfaces. My question here is: I will have to package RecordNotFoundException with the Cleint.jar file to make it available to the client right. Is there any issue concerning serialization (implements serializable) for this exception so that it can travel across the network wire? Your quote:

I think we could argue that it is a FatalSystemException. After all, the clients we are developing should only ever try to gain on]e lock. If they try to gain more locks, then there are a whole bunch of issues that we have not catered for (and are not required in the specifications). So if they do try to get more than one lock, then they are going to cause the system to go into a state that has not been coded / tested. However this may be a case where you want to catch the FatalSystemException in your book() method, determine that it occured because of attempting to lock more than one record, and just log it server side (don't shut server down) and then pass it up to the client. The server trapped it before it got into the invalid state, so you should be safe to carry on.

Based on your feedback above, here is how I have modified my code: First, the lock method in the Data class:

Next, the book method in the DataAdapter class

Do I have everything right? I appreciate your walking me thorough this effort. G'day to you in Sydney Australia. It is around midnight in Boston, Massachusetts. Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Philippe Maquet

Bartender

Posts: 1872

posted 13 years ago

Hi Bharat, I have two comments about the lock() code above :

Why do you prevent clients to own more than one lock at a time ?

In your HashMap, recNo is the value. But as HashMap.containsValue() performs a sequential search, it may be very slow.

So, why don't you put recNos as keys and instanceRefs as values. If you allow multiple locks owned by a client, you wouldn't need to call containsValue() at all and your while() loop would be speed up by replacing the containsValue() by a containsKey(). Now in your boolean book method, I don't see any other possible return value but true (false cannot be returned). So this method could be a void one. Best, Phil. [ September 09, 2003: Message edited by: Philippe Maquet ] [ September 09, 2003: Message edited by: Philippe Maquet ]

Hello Andrew, Thanks for walking me through the exception handling piece. Next I am going to follow through on Phil's comments on my locking strategy. Please stay with us and comment whenever you can. Hello Phil, You wrote:

Now in your boolean book method, I don't see any other possible return value but true (false cannot be returned). Si this method could be a void one.

Based on your feedback above, I have modified my book method in the DataAdapter class as follows:

You wrote:

Why do you prevent clients to own more than one lock at a time

I believe that you are referring to the following piece of code in the lock method in Data class:

First, you are right, this will prevent the client from locking any "other" records once it has acquired a lock on "any" record contained in the data file. Let us think about it, should it allow the client to do so though? The reasons that I decided to do so were: 1. The assignment certainly doesn't prevent me from doing that does it? Further, it simplifies my work. - I can document it in my submission? 2. In the course of a normal client interaction with the system, should it even be allowed to acquire multiple records locks? Consider this, we have a book method in the DataAdapter class which uses the services of lock and unlock methods in its operation. In this time, the client should not be doing anything else since this is a button click on the "book" button in the GUI. Similarly, any other "future" update type transactions should only be allowed one at a time. My find method is synchronized as follows: public synchronized int [] find(String [] criteria) throws RecordNotFoundException { Therefore, it bypasses the lock method all together. I envison allowing the client only "one" data mutation type operation at a time whereas they are free to retrieve records as and when needed. Does that sound right? You wrote:

In your HashMap, recNo is the value. But as HashMap.containsValue() performs a sequential search, it may be very slow. So, why don't you put recNos as keys and instanceRefs as values. If you allow multiple locks owned by a client, you wouldn't need to call containsValue() at all and your while() loop would be speed up by replacing the containsValue() by a containsKey().

Believe me, that is what I wanted to do initially. The reason I had to "flip" my key/value pair is that my lock, unlock, and islocked method signatures in the DBMain interface are as follows:

Which means that I cannot count on using a cookie value to unlock a locked record by the locking client. The only way that I could "encapsulate" the locking behavior which respects the requirement: "Locks a record so that it can only be updated or deleted by this client" was by recording the unique id of the data instance by taking the value of Data.this as Max suggested in the other thread and storing the value of record number to be locked. This way, the WeakHashMap would loose the reference to this client if the client crashed! The problem that I kept running into with lock, unlock, and islocked if I used recNo as the key was that the method signatures require the record Number to be passed as an "int". Now, if you want to store this as the key for the WeakHashMap then you have to do a "wrap" operation inside the methods as Integer(recNo) and then use it since collections don't allow primitives to be stored as primitives instead you have to wrap them first. Since the wrapping happens inside these methods, the WeakHashMap immediately releases the row for the locking client, since I use the following code in my DataAdapter class in the book method:

I am not sure how to get around this problem? I did a search on the locking stuff and found the following thread where Max suggested to flip the key/value pairs around. So that is what I did.How to Lock(-1) In this thread Max quotes:

Nate, This part might have been overkill, depending on your design. If each client gets thier own thread(as in, say, a factory based RMI network approach), then you can use a WeakHashMap to store locked records in. As you know, the value of a WeakHashMap is that when a key object loses all references to it, it is released, as is the value object. The elegence of this approach comes in using the Thread itself as the key for the WeakhashMap, and the recordID as the value. This way, when a thread dies, it will eventually release the record. Thus, there's no need for a reaper thread. HTH, M, author

Any suggestions that you, Andrew, Max or anybody else have are welcome. Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Philippe Maquet

Bartender

Posts: 1872

posted 13 years ago

Hi Bharat, I hope I won't put some confusion in the thread here, and sorry if I do.

1. The assignment certainly doesn't prevent me from doing that does it? Further, it simplifies my work. - I can document it in my submission?

Yes, nothing in the instructions explicitely prevents you to allow only one record to be locked at a time by a given client, and you may document your decision. So far so good. But it depends on how you interpret this sentence : "It must allow the user to book a selected record, ...". Does "a" means "one" for sure ? If you reply "yes", your design is OK ... for the moment. If you reply "no" or "maybe", your locking design is too restrictive. Personally, I replied "maybe" to that question, so - if I feel lazy - I'll probably allow only one record to be selected (and booked) and a time client-side. If less lazy, I'll set a property like "Allow multiple selection (Y/N)". But server-side my book method will accept multiple records to be booked together. After all, even one record may be seen as a singleton collection, right ? The pros are obvious (it's quite typical for a customer to want to book two nights together, making sure both nights are available).

Which means that I cannot count on using a cookie value to unlock a locked record by the locking client.

I would say that you are lucky to have a version of the assignment with no cookie value ! In my locking implementation, I must generate them and check them without any benefit... Another issue I'd like to point out (it's not the first time, but I'd like to repeat it here), is that the static HashMap solution (I suppose it is static), "Weak" or regular, as shown in so many posts here, is incompatible with a multiple-tables design (because a recNO alone cannot identify a record if there are more than one table). We only need to support one db file in the assignment, so it's OK, but to be defendable reasonably, it should be argued that there will not be additional tables in the future. IMO, it's difficult to defend : customers IDs are known by the CSR's "from memory". Is that so improbable that those guys ask in the future some "pickup" facility of customers IDs from a customers.db file ? Worst, they tell us in the instructions that they intend to move the application to the web. So we know today that multiple tables will have to be supported.

In this thread Max quotes: quote: -------------------------------------------------------------------------------- Nate, This part might have been overkill, depending on your design. If each client gets thier own thread(as in, say, a factory based RMI network approach), then you can use a WeakHashMap to store locked records in. As you know, the value of a WeakHashMap is that when a key object loses all references to it, it is released, as is the value object. The elegence of this approach comes in using the Thread itself as the key for the WeakhashMap, and the recordID as the value. This way, when a thread dies, it will eventually release the record. Thus, there's no need for a reaper thread. HTH, M, author --------------------------------------------------------------------------------

Here, Max surprises me ! The thread as the lock owner ? That's what I did in my first design and he criticized it just for that reason ?! I must miss something here. Moreover, I still don't understand how that WeakHashMap solution will work (see this thread). There is a (lack of) notification issue ! If you read back your lock() code, you are waiting for a record to be available for locking : lockedRecords.wait(). When the WeakHashMap will be cleared of those entries belonging to a crashed client, it will be done silently, with no notification of any sort. You could solve the issue by replacing the wait() by a wait(timeout), but would it be in accordance with the instructions which state "the current thread gives up the CPU and consumes no CPU cycles until the record is unlocked" ? No, IMO. I repeat that a solution based on WeakReferences registred with a ReferenceQueue and stored in regular HashMap is hardly more complex and ... works ! Andrew, do you agree with that ? Best, Philippe. [ September 09, 2003: Message edited by: Philippe Maquet ]

Bharat Ruparel

Ranch Hand

Posts: 493

posted 13 years ago

Hey Phil, Thanks for quick response. You wrote:

If you read back your lock() code, you are waiting for a record to be available for locking : lockedRecords.wait(). When the WeakHashMap will be cleared of those entries belonging to a crashed client, it will be done silently, with no notification of any sort. You could solve the issue by replacing the wait() by a wait(timeout), but would it be in accordance with the instructions which state "the current thread gives up the CPU and consumes no CPU cycles until the record is unlocked" ? No, IMO. [B] I repeat that a solution based on WeakReferences registred with a ReferenceQueue and stored in regular HashMap is hardly more complex and ... works ! [B]

This sounds interesting. Can you give me some references/places I start looking? Thanks. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Philippe Maquet

Bartender

Posts: 1872

posted 13 years ago

Hi Bharat,

This sounds interesting. Can you give me some references/places I start looking?

Read carefully the WeakHashMapdoc, and try to find a way to get notified when one of its entries is cleared. IMO, you won't find one . Now have a look at WeakReference and ReferenceQueue docs and ... the light will be. There is no magic with WeakHashMap. It's not the garbage collector which clears its entries. As far as I know, it stores its entries as WeakReferences registred with an internal ReferenceQueue of its own, which it polls on some operations to check is some of its entries have to be cleared : WeakHashMap clears its entries by itself ! Have a look to WeakHashMap sources, and you'll see how it's simple. But remember just this : you cannot wait forever in your lock method. When unlocking by yourself, you notify() the waiting threads ... perfect. But when when some "indirect" unlock is performed because of a client crash, if the client waiting of such a lock is not notified by some way, it will simply hang. It will be nice when Max will tell us his view on the problem I think. Best, Phil.

Max Habibi

town drunk( and author)
Sheriff

Posts: 4118

posted 13 years ago

Hi Guys, A couple of quick points here. I'm a firm believer in the XP/Agile school of design. Therefore, I don't think it's a good idea to design to non-existent requirements: my motto is to do the simplest thing that will work. It follows, therefore, the I don't think you need to create a solution that supports multiple tables, because Sun's requirements don't ask you do to so. Of course, it could be argued, as Phil elegantly does, that future enhancements might require supporting more tables. It could also be argued, however, that future enhancements might require a real database, like Oracle or MySQL. By coding and designing to support things that the client didn't ask for, it could be argued that you're not being strickly professionally ethical. That is, if this were a real project, I'm not sure you have the right to use the client's time and money in order to provide feature they didn't ask for. How would you feel if your local mechanic did this the next time you took your car in for an oil change? Further, the requirements for this assignment don't require that you deal with client crashes, or other abnormal termination issues. To the point, they don't require that you deal with timeout of locks. Sun simply wants to know if you can write Thread safe code, as described in the requirements. If you can demonstrate that much, then you've done what they asked for. They will not be impressed by additional features, etc. They don't care about them. It's just more code that they will have to wade through to figure out what you did. Put yourself in the grader's position for a minute. They want to see conventional solutions, they want easy-to-follow code, they want get done grading your assignment earlier then they had expected, and they want to go home. My suggestion is that you focus on the hard requirements, get them 100%, and then start to consider enhancements, if any. Of course, this is your assignment, and you should do whatever you think is right. All best, M

Originally posted by Philippe Maquet: Hi Bharat, I hope I won't put some confusion in the thread here, and sorry if I do.

Here, Max surprises me ! The thread as the lock owner ? That's what I did in my first design and he criticized it just for that reason ?! I must miss something here. [ September 09, 2003: Message edited by: Philippe Maquet ]

No, I was just wrong above. I've been tempted to go back and delete that post, but I don't really care for that on principle. M

Hello Max, Now I am confused! Are you saying that you were wrong in suggesting to store Data.this as the key for the static WeakHashMap and recNo as its value or you were wrong in your criticism? If storing of "Data.this" as key is wrong then my locking class is incorrect! I don't think so because it seems to work, but I haven't tested it thoroughly yet. Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Philippe Maquet

Bartender

Posts: 1872

posted 13 years ago

Hi Max, I agree with the main parts of your posts. But with these little nuances though :

By coding and designing to support things that the client didn't ask for, it could be argued that you're not being strickly professionally ethical. That is, if this were a real project, I'm not sure you have the right to use the client's time and money in order to provide feature they didn't ask for.

You are 100% right if this were a real project. But in such one, you would not have such a statement in your instructions :

This document deliberately leaves some issues unspecified, and some problems unraised. Your ability to think through these issues, in the face of realistically imperfect specifications, and come to a tenable solution is something upon which you are being graded.

I understand that they explicitly ask us to be reasonably creative to solve the issues they deliberately left unspecified ... and we are graded in that too. So it's not so easy IMO to know exactly where to stop in solving such unspecified issues.

Further, the requirements for this assignment don't require that you deal with client crashes, or other abnormal termination issues. To the point, they don't require that you deal with timeout of locks. Sun simply wants to know if you can write Thread safe code, as described in the requirements.

Agreed, with the same nuance. But anyway, if you do such additional things, they must work. I mean that if a static WeakHashMap specifically choosen as a solution to handle crashed clients don't work (see my comments in previous posts in this thread about the lack of notification), it's far better IMO use a regular HashMap instead and to "forget" the issue completely. As using a WeakHashMap there needs to be justified (in choices.txt), you cannot afford any unexpected wrong behaviour. Here we should agree totally : less is far better than wrong.

Put yourself in the grader's position for a minute. They want to see conventional solutions, they want easy-to-follow code, they want get done grading your assignment earlier then they had expected, and they want to go home.

That part of your post makes me afraid. Afraid to have written too much code based on too complex designs ... giving more job to the grader which could lead him to feel me as a quite antipathic guy. On the other hand, I suppose that graders don't read everything : I hope that after having read choices.txt, they choose parts of your source code which interest them more than other ones, in such a way that the global volume of your code would not be a real issue. Anyway I hope so, because if it's not the case, I am going straight in big troubles. Best, Phil. [ September 09, 2003: Message edited by: Philippe Maquet ]

Max Habibi

town drunk( and author)
Sheriff

Posts: 4118

posted 13 years ago

Originally posted by Bharat Ruparel: Hello Max, Now I am confused! Are you saying that you were wrong in suggesting to store Data.this as the key for the static WeakHashMap and recNo as its value or you were wrong in your criticism? If storing of "Data.this" as key is wrong then my locking class is incorrect! I don't think so because it seems to work, but I haven't tested it thoroughly yet. Regards. Bharat

No, I was wrong to suggest using the current thead. Data.this is perfect. M

Hi Phil, It's good to talk to again: and thanks for pitching in to help newer people as much as you have been. Andrew and I both appreciate it

Hi Max, I agree with the main parts of your posts. But with these little nuances though :

quote: -------------------------------------------------------------------------------- By coding and designing to support things that the client didn't ask for, it could be argued that you're not being strickly professionally ethical. That is, if this were a real project, I'm not sure you have the right to use the client's time and money in order to provide feature they didn't ask for. -------------------------------------------------------------------------------- You are 100% right if this were a real project. But in such one, you would not have such a statement in your instructions :

quote: -------------------------------------------------------------------------------- This document deliberately leaves some issues unspecified, and some problems unraised. Your ability to think through these issues, in the face of realistically imperfect specifications, and come to a tenable solution is something upon which you are being graded. --------------------------------------------------------------------------------

In the absence of a request, I don't think I would perform a service

I understand that they explicitly ask us to be reasonably creative to solve the issues they deliberately left unspecified ... and we are graded in that too. So it's not so easy IMO to know exactly where to stop in solving such unspecified issues.

quote: -------------------------------------------------------------------------------- Further, the requirements for this assignment don't require that you deal with client crashes, or other abnormal termination issues. To the point, they don't require that you deal with timeout of locks. Sun simply wants to know if you can write Thread safe code, as described in the requirements. -------------------------------------------------------------------------------- Agreed, with the same nuance. But anyway, if you do such additional things, they must work. I mean that if a static WeakHashMap specifically chosen as a solution to handle crashed clients don't work (see my comments in previous posts in this thread about the lack of notification), it's far better IMO use a regular HashMap instead and to "forget" the issue completely. As using a WeakHashMap there needs to be justified (in choices.txt), you cannot afford any unexpected wrong behaviour. Here we should agree totally : less is far better than wrong.

Agreed, if they don't work. However, I submit that making it work is an implementation issue . I don't really want to go down this path, because you're correct that this is not a requirement, and it can be distracting to people who are just begining to find thier way. However, I hope you believe me when I say my opinion is that WeakHashMap is just as failsafe as any other method, if not more so. I may, of course, be mistaken.

quote: -------------------------------------------------------------------------------- Put yourself in the grader's position for a minute. They want to see conventional solutions, they want easy-to-follow code, they want get done grading your assignment earlier then they had expected, and they want to go home. -------------------------------------------------------------------------------- That part of your post makes me afraid. Afraid to have written too much code based on too complex designs ... giving more job to the grader which could lead him to feel me as a quite antipathic guy. On the other hand, I suppose that graders don't read everything : I hope that after having read choices.txt, they choose parts of your source code which interest them more than other ones, in such a way that the global volume of your code would not be a real issue. Anyway I hope so, because if it's not the case, I am going straight in big troubles. Best, Phil.

It's a valid concern. OTOH, I'm sure you're not the first person to choose a socket implementation. And given how well you articulate you point on this forum, I'm sure you'll be fine All best, M

Personally I don't think so. I believe that on application startup (both stand alone client and server) you should verify the cookie. Even if you do nothing else and then close the application. From then onwards you should be safe with cookie - the instructions tell you that no other application will modify the file while your application owns it. So you could probably have the check either in the Data constructor (in which case you can throw any exception you like) or in your own method in the Data class (again you can throw any exception you like). You should not need to chain exceptions in this case. (As an aside: You do (IMHO) want this check on application start up. There would be nothing worse than starting the server application, going home before anyone tries to do a booking, and later that the server crashed the first time someone tried to do the booking all because you had an invalid data file and it wasnt checked at startup. Much better to do a simple check at application startup and shut the server down instantly if the file is wrong.)

Based on what I learned from you on exception handling, here is what I did: I have coded a custom MagicCookieException class as follows:

In my Data class constructor, I check for the magic cookie and throw the MagicCookieException if the value doesn't match: ]

Now I don't do anything with it and let it bubble all the way up to my MainWindow class which displays the JTable and use the ApplicationRunner class's handleException method (thank you Max) in the MainWindow' constructor to catch it as follows:

This works like a charm! The alert box displays the message "Magic cookie value check failed" that I pass to it when I throw the exception. Next, the system exits. The only thing I am not sure about is that my console displays the stack trace for this exception while it displays the alert box. Since this is a show-stopper, I think it is OK to do so. What do you think? I want to handle InterruptedException which is rethrown as a FatalSystemException as well as the FatalSystemException thrown while trying to lock and already locked record as we discussed above in a similar manner. Will appreciate your feedback. By the way, I am not sure at this point if it is advisable to pursue refining the locking stuff further based on Max's comments. I am inclined to leave it as it is provided that it is correct. What do you think? Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Philippe Maquet

Bartender

Posts: 1872

posted 13 years ago

Hi Max,

It's good to talk to again

A shared feeling !

It's a valid concern. OTOH, I'm sure you're not the first person to choose a socket implementation. And given how well you articulate you point on this forum, I'm sure you'll be fine

Hi Everyone, Bharat - looking at the book() method you posted above. You could try to minimise the scope of your "String[] data" and "String msg" variables. The first is only used in the try block and the second only in the catch block.

[Philippe] But it depends on how you interpret this sentence : "It must allow the user to book a selected record, ...". Does "a" means "one" for sure ? If you reply "yes", your design is OK ... for the moment. If you reply "no" or "maybe", your locking design is too restrictive.

Personally I do read the "a selected record" as one record. It is not "some records" or any other plural form.

[Philippe] the instructions that they intend to move the application to the web

True, but the instructions also tell us that "The IT director does not anticipate much reuse of the first Java technology system". So as Max says, we should not put too much effort into handling non existant requirements. By all means, make sure your code can be reused, but don't add functionality that is not required.

[Philippe] I repeat that a solution based on WeakReferences registred with a ReferenceQueue and stored in regular HashMap is hardly more complex and ... works !

But a ReferenceQueue still does not have any callback facility. So you still need some way of checking if anything is in the ReferenceQueue - possibly a separate thread. In which case why not have a separate static variable containing the count of items that you have explicitly added / removed from your WeakHashMap? Your thread could periodically compare that number with the real number that is in the map, and if they differ, update the count and call notifyAll(). The only advantage the ReferenceQueue gives you is if you want to wake only those threads explicitly waiting for a specific lock.

[Max to Phil] thanks for pitching in to help newer people as much as you have been. Andrew and I both appreciate it

Yes, I also want to add my thanks. Likewise to Bharat, and all the others who also help out. It is great to see this happening.

[Bharat] (after throwing MagicCookieException) The only thing I am not sure about is that my console displays the stack trace for this exception while it displays the alert box. Since this is a show-stopper, I think it is OK to do so.

I agree. If you had logging in your solution then you might want to send the stack trace to the logger. The client would then be able to send the log file to you so you can see exactly what went wrong. Much better than trying to work out what the client means when they phone you to say that "the application crashed because of some bad snack food". Logging is not a requirement for this assignment, but it is very easy to implement. I have not yet deployed a live application that did not have some form of logging included.

By the way, I am not sure at this point if it is advisable to pursue refining the locking stuff further based on Max's comments. I am inclined to leave it as it is provided that it is correct. What do you think?

I was looking at one of the recent posts by someone who got 100% in their locking and server design. And they had a very simple design, with no handling of deadlock or dead clients. So certainly all the complexity that we often discuss is not required. Regards, Andrew

Personally I do read the "a selected record" as one record. It is not "some records" or any other plural form.

OK, I just thought that as there is a one-to-one relationship between a "book" and a record (update its owner field), it just means that the user must be able to book a selected recorded. I would be 100% confident in your interpration if "a" had been replaced by "the". A selected record simply is a record which is selected among other ones which could be selected or not. Of course, if you allow mutiselect in your JTable and your book() methods (as your locking scheme) only allows one record to be booked at a time, it's still possible to call book() in a loop. But you losses the transacional benefit and you increases network traffic. But anyway you are right : if your database design doesn't allow it, there is no issue (I mean it's OK for you Bharat, Andrew is right). But in my case, as my locking scheme allows multiple locks (and it we be stupid of me to invest one hour of my time to refactor LockManager in order to loose that functionality ! ), I probably may keep my book() method allowing a collection of records to be booked at once while setting optional the multiselect property of the JTable.

True, but the instructions also tell us that "The IT director does not anticipate much reuse of the first Java technology system".

Right. Unfortunately, they don't tell us in which parts reuse is not expected (database ? network ? GUI ?) and /or to which extend : does "not much" mean 10%, 30%, 50%, 70% ? There are no answer to both thise questions in the instructions.

quote: -------------------------------------------------------------------------------- [Philippe] I repeat that a solution based on WeakReferences registred with a ReferenceQueue and stored in regular HashMap is hardly more complex and ... works ! -------------------------------------------------------------------------------- But a ReferenceQueue still does not have any callback facility. So you still need some way of checking if anything is in the ReferenceQueue - possibly a separate thread.

More than possibly : you need one. But as I have such a maintenance thread running in LockManager for other purposes already (timeouts management), polling the ReferenceQueue is just 4 lines of code (including curly braces ) in its run() method.

In which case why not have a separate static variable containing the count of items that you have explicitly added / removed from your WeakHashMap? Your thread could periodically compare that number with the real number that is in the map, and if they differ, update the count and call notifyAll().

Great ! You come here with a very nice, simple and elegant solution, Andrew. I love it ! Now Max's static WeakHashMap solution to locking, perfect for a simple locking design, finally may work. As i wrote in a previous post, less is better than wrong, and I am pretty sure that some people who implemented the WeakHashMap solution willing legitimately handle client crashes, were not aware of the fact that other waiting clients might hang forever, so delevering a buggy solution. We should remember your static locksCount solution for future posts on the same issue.

Your thread could periodically compare that number with the real number that is in the map, and if they differ, update the count and call notifyAll().

Yes, but as you notice yourself, you need some maintenance thread too, but with just a few lines in its run() method. It's perfect.

The only advantage the ReferenceQueue gives you is if you want to wake only those threads explicitly waiting for a specific lock.

Yes, indeed, but in my solution (or a comparable one) only. In Bharat's solution (like most solutions I've read on this forum), callers are waiting on the HashMap. They must be notified by a notifyAll(), all of them then compete just to acquire exclusive access to the HashMap again, and then still need a containsKey() (or even a containsValue() in Bharat's design) before getting just a chance to acquire the lock. In my implementation (a HashMap of Records as keys, each of them having as value a LinkedList of Lock objects), it's quite different. I grant locks in FIFO order. For any record (table/recNo pair) (if present in the HashMap as a key), the first Lock object in the list is a granted lock. The next ones are claimed locks on which the callers are waiting to be notified. So, unlocking a record simply means removing the first Lock in the list, then if there is a next one, notify() (and not notifyAll()) the caller, else removing the whole entry from the HashMap. It is probably more efficient. Best, Phil.

We should remember your static locksCount solution for future posts on the same issue

Yes, but we don't want to present this solution the first time someone mentions that they are using a WeakHashMap. Let them think about the issue first, and come to their own solution. We don't want to spoil anyone else's fun in finding these solutions do we? Regards, Andrew

If you had logging in your solution then you might want to send the stack trace to the logger. The client would then be able to send the log file to you so you can see exactly what went wrong. Much better than trying to work out what the client means when they phone you to say that "the application crashed because of some bad snack food". Logging is not a requirement for this assignment, but it is very easy to implement. I have not yet deployed a live application that did not have some form of logging included.

Will do. Thanks. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Ulrich Heeger

Ranch Hand

Posts: 266

posted 13 years ago

Hi everyone, I have a few questions concerning certain points in this thread and its preceding one, NX: About data consistent.(I'm afraid some of my questions sounds a little bit stupid, so excuse me 1. Max quote:

Just as an FYI, I would argue that the InteruptedException should cause an explicit fatal exception to thrown: one that ripples up to middle tier as a FatalSystemException, and eventually to the user as a FatalGUIException, and which cause the system to shutdown. This is how most apps deal with this sort of thing in the real world.

Should the server shutdown or only the client? 2. Andrew quote:

Personally I don't think so. I believe that on application startup (both stand alone client and server) you should verify the cookie. Even if you do nothing else and then close the application. From then onwards you should be safe with cookie - the instructions tell you that no other application will modify the file while your application owns it. So you could probably have the check either in the Data constructor (in which case you can throw any exception you like) or in your own method in the Data class (again you can throw any exception you like). You should not need to chain exceptions in this case.

Does it mean, that for example when I start the server for the networking mode, there must be a call to read the db.file in order to store the Metadatas in static members (either in primitive variables or within an own DBSchema-class)? So, when a Data-Class instance is created, the read MagicCookie is checked against the static value? Regards, Ulrich

Tony Collins

Ranch Hand

Posts: 435

posted 13 years ago

Hi Ulrich, 1 ) An InteruptedException on the Server side means there is a bug in the server, in my design anyway. So I ripple a FatalError up ( how far is your own choice), log the error and exit the server. You could throw a Exception to the client if you wanted but you'd have to give the corrupted server some time to send it. 2) At start up you you would check the magic cookie when tou collected the metadata from the files header. Some of us actually cache at this momment at once.

Tony

Bharat Ruparel

Ranch Hand

Posts: 493

posted 13 years ago

Hello Ulrich, You wrote:

Does it mean, that for example when I start the server for the networking mode, there must be a call to read the db.file in order to store the Metadatas in static members (either in primitive variables or within an own DBSchema-class)? So, when a Data-Class instance is created, the read MagicCookie is checked against the static value?

So I ripple a FatalError up ( how far is your own choice), log the error and exit the server. You could throw a Exception to the client if you wanted but you'd have to give the corrupted server some time to send it.

Sorry, sometimes I have a little problem with my English, does it mean, the client exits the server or the server itself shuts down? And if the server shuts down, the clients should be notified that the server has to be restarted? Hi Bharat, thank you for the link. I have one more question, further up in this thread you'were speaking about the FatalSystemException which you want to keep in a seperate package(suncertify.exceptions). Is'nt that opposed to the requirements, in which it's written:

Any unimplemented exceptions in this interface must all be created as member classes of the suncertify.db package. Each must have a zero argument constructor and a second constructor that takes a String that serves as the exception's description.

Hello Ulrich, I have the same instructions that you have. I was interpreting them as that we have to create 1. RecordNotFoundException, and 2. DuplicateKeyException in the package "subcertify.db". Further, they should have a no-arg constructor and a constructor that takes String parameter. These are by definition, checked exceptions since we have been given the interface which delclares them as being thrown from various methods. The exceptions that I have created, e.g., FatalSystemException and MagicCookieException extend RuntimeException and therefore they don't need to be declared in the "throws" clause of the method. They can be caught though. Moreover, my GUIController catches these exceptions and sends the alert to the user. This means that these exceptions' defintion must be available at all the layers (more specifically where they are thrown and where they are caught). This means the data layer and the GUI layer at-least. How is it possible to just declare them in suncertify.db package level then? I may be wrong. May be Andrew/Max/Tony/Phil/Vlad can confirm for us? Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Bharat Ruparel

Ranch Hand

Posts: 493

posted 13 years ago

Hello Max, You wrote:

I think the server: The error happened on the server, and it affects every client. It's a legitimate time for a system crash, IMO.

How do you shut the server down? Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Ulrich Heeger

Ranch Hand

Posts: 266

posted 13 years ago

Hi Max, hi Bharat, thanks for your replies Bharat, you wrote:

How is it possible to just declare them in suncertify.db package level then?

I think you're right, I was just a little bit anxious, but I share your opinion

Hello Max, You wrote: quote: -------------------------------------------------------------------------------- I think the server: The error happened on the server, and it affects every client. It's a legitimate time for a system crash, IMO. -------------------------------------------------------------------------------- How do you shut the server down?

Perhaps server-side within the catch-clause with System.exit(1)? Regards, Ulrich

Well, yes. That would be the point . If the server goes, the client will get a RMI connection exception, which they can, in turn, treat as a TerribleException, etc. Or, you could throw a TerribleException and start a Thread to shut down the server. Either works, though the latter can be messier. M [ September 15, 2003: Message edited by: Max Habibi ]

Hi Max, thanks for your reply, now I can go on But I have just one more question.

If the server goes, the client will get a RMI connection exception, which they can, in turn, treat as a TerribleException, etc. Or, you could throw a TerribleException and start a Thread to shut down the server. Either works, though the latter can be messier.

1.How do you think the client should handle the TerribleException? I think, the client should get a message that his application will shut down, perhaps the current thread should sleep a while so the client can be aware of the message's content (and realize what's going on ) and after that the application shuts down. What do you think about this idea?2.In the standalone mode, when the networking mode is bypassed, can the InterruptedException occur? I would say no, because we don't have concurrent threads as long as I don't explicitly implement a second one. I'm right? So, I don't need to care about this Exception in this mode? Thank you very much in advance, Ulrich

Bharat Ruparel

Ranch Hand

Posts: 493

posted 13 years ago

Hello Ulrich, You wrote:

In the standalone mode, when the networking mode is bypassed, can the InterruptedException occur? I would say no, because we don't have concurrent threads as long as I don't explicitly implement a second one. I'm right? So, I don't need to care about this Exception in this mode?

In theory, the InterruptedException should never occur in standalone mode. However, there are two motivating forces for taking it into consideration: 1. As much as you can, maintain the same code base for networked as well as standalone mode. Even if some of the locking/exception handling code is redundant, keep it there. This simplifies the maintenance process. In my opinion, whenever I have kept things simple, they have tended to work. Conversely, I am amazed how much the maintenance work increases when we complicate the things just a little bit. 2. I thought that whole motivating force for starting this thread was dealing with situations where "things" happen that are "drastic" in nature. Hence the name "FatalSystemException" in the first place! Under normal circustances, in the standalone mode, this should never happen. For that matter, if your code is right and you have designed your system so that each client has one instance of the Data class (my locking strategy is based on that premise), you still should never get that exception. The way I understand it (and I may be wrong!), whenever you sub-class the RuntimeException class, you are implicitly acknowledging that these "types" of exceptions are rather "drastic" and "unusual" but should they occur anyway, here is how you have chosen to deal with them. Therefore, I have chosen to handle the InterruptedException by sub-classing the RuntimeException and let it bubble all the way up to the client telling him that "bad stuff" is happening in the locking method of the Data class and please contact the system admin right away! I use exception chaining mechanism which Andrew taught me here and Max suggested. Again, I can be corrected but this is the current state of my knowledge of Exception Handling as applicable to the assignment. By all means, run with the tread. I appreciate your questions and follow-ups. Regards. Bharat

SCJP,SCJD,SCWCD,SCBCD,SCDJWS,SCEA

Max Habibi

town drunk( and author)
Sheriff

Posts: 4118

posted 13 years ago

Originally posted by Ulrich Heeger: Hi Max, thanks for your reply, now I can go on But I have just one more question.

1.How do you think the client should handle the TerribleException? I think, the client should get a message that his application will shut down, perhaps the current thread should sleep a while so the client can be aware of the message's content (and realize what's going on ) and after that the application shuts down. What do you think about this idea?

, but you don't need to sleep. Just pop up a message and let the client block until the customer presses ok. Then, do a system.exit.

2.In the standalone mode, when the networking mode is bypassed, can the InterruptedException occur? I would say no, because we don't have concurrent threads as long as I don't explicitly implement a second one. I'm right? So, I don't need to care about this Exception in this mode? Thank you very much in advance, Ulrich

I would think yes. For example, if you're not using a cache, then an IO exception will cause an InterruptedException, I think. Frankly, I would react the same way. Have the lock/unlock catch the InterruptedException, wrap it in some other RuntimeException(say, ReallyBaDThingHappendedException), and throw it to the client. The client can catch that and go from there. Why wrap it? Because, IMO, the client doesn't need to know that there was any threading or IO going on. InterruptedException is sufficient for the needs of the person sitting behind the counter, and if you chain your InterruptedException into your ReallyBaDThingHappendedException, the logs will have enough details to be useful. M [ September 16, 2003: Message edited by: Max Habibi ]

but you don't need to sleep. Just pop up a message and let the client block until the customer presses ok. Then, do a system.exit.

That is what I am doing. The only trouble that I have is by the time I can pop-up this "alert" window, I am already in the GUI layer. The client exits fine and shows an appropriate stack-trace. I am a bit mystified about shutting down the server. Do you mean to say when you catch the InterruptedException in the lock method of the Data class, do the following: 1. Wrap it into a sub-class of RuntimeException, e.g., FatalSystemException and throw it as a chained exception. AND RIGHT AFTER THAT; 2. Issue a System.exit(1) in the lock method of the Data file to shut the "Server" down? Will appreciate your response. Thanks. Bharat