Error handling considerations

[chronozphere]

Hi everyone.

I was looking into the asphyre source. And i must admit, that it's an impressive piece of work. The code looks very clean and it all fits together in a very elegant way. :)

One thing i noticed was that there wasn't much error handling code present.
Only error checking like this:


if not Succeeded( ARiskyAPICall) then
exit;


Okay... This is the way i do it:


Result := ARiskyAPICall;
if not Succeeded(Result) then
begin
ReportError( Result, 'A Risky API Call failed' , CurrentModule);
Exit;
end;


Report error assembles the information and adds it to the log-file.

My approach results in bloated routines. It takes 6 lines instead of 2 and therefore it is much work to handle every error in the entire engine and give them all nice descriptive error-strings. The Asphyre way looks much better and cleaner. ;)

So i started thinking.... Do i need all this error-handling code :? ?

When i throw away my error-handling, I will get much cleaner code and i dont have to think about the errorstrings. So generally: maintaining your code will be easier but debugging it might be harder.

I like to know your experiences with this issue.

What is your approach?
When do you handle/log your errors and when not?
Do you find it easy to debug code with Asphyre-Style error checking?

Thanx a bunch! ;)

[technomage]

Personally, I think error handling is important. If another develper is using your tools/libary and it doesn't do what it's expecting yuo should really tell them by way of results (being false) or via raising exceptions.

As for a user, resonalble error handling is good, a normal user getting a highly technical error message is just going to get confused.

so I guess the answer depends on who you are writing the code for..

[LP]

There are several ways to handle errors. One way is to throw an exception, access violation or reboot the system. Another way is to determine the best course of action when the error occurs so the system continues to function.

This is IMHO an example of bad VCL design. I've seen in several occasions applications throwing "Index out of bounds" error (they forgot to disable exceptions in retail build), which shouldn't really occur.

If you open "Vectors2px.pas" (a unit, which provides TPoint2px record - the same as TPoint, but with operator overloads and a list of TPoint2px entries), check "TPoints2px.GetItem" routine:


function TPoints2px.GetItem(Num: Integer): PPoint2px;
begin
if &#40;Num >= 0&#41;and&#40;Num < DataCount&#41; then Result&#58;= @Data&#91;Num&#93;
else Result&#58;= nil;
end;


The above code checks whether the index is inside the range and returns the appropriate value, instead of just throwing the error, so the end application can determine what to do with it.

In some places Asphyre uses "silent error handling", which is essential in Direct3D app, instead of throwing exception and locking the application in full-screen mode, so user has to open Task Manager to close the app.

In some places, simple error checking is made by returning boolean. While developing Direct3D app, you should use Debug Runtimes to get extended error information. However, there is no need to show these detailed reports to your end users, just tell them to update DirectX to 9.0c, install latest video drivers and make sure they have hardware accelerated video card.

[{MSX}]

I think IMHO that the way of handling error of getItem (returning nil) is not good :)
First, it doesn't solve the problem of having to check every function returning something:

a := getItem()
if a==nil then
begin
log();
exit;
end;
b := getItem()
if b==nil then
begin
log();
exit;
end;
c := getItem()
if c==nil then
begin
log();
exit;
end;


Second, returning nil will likely throw an access violation (nil referencing) if you forget to check the result before using it. The AV can sometimes be triky to track down. And if you don't forget to check the result, maybe your fellow coders will do :)

I think the right way to do error checking is with Exceptions. They were indeed invented for this reason. I learned to love them in Java :)

For example, you can do this:


try
a := getItem();
b := getItem();
c := getItem();
except
log();
end;


That looks way better. Even more, you don't need a try except if the caller have its own since the exceptions propagate to the first except block in the stack.


procedure wasteTime();
begin
a := getItem();
b := getItem();
c := getItem();
end;


This mean that you can just get rid of 90% of the "if error then log exit" stuff. I've seen by experience that a couple of try/except well placed around can make all the job.
Using diffenent Exception subclasses helps a lot: for example one that you catch inside a loop, and one more severe that propagate to the main.

The problem of the "if error then log and exit" is that it grows with every call. And call inside call needs them too, and so on. That way a program almost grows of three times.
With exceptions, you don't need a check for each call, and not even one for each function.

The indexOutOfBound exception is not that bad IMHO. You can use exceptions just as IFs:


a=accessArray(i);
if a=nil then
begin
log;
exit;
end
else
begin
doStuff(a);
end;


try
a:=accessArray(i);
doStuff(a);
except IndexOutOfBound
log();
end;

I do this only if i handle all necessary stuff in the except block. If i have to report the fact to the caller, i let the exception propagate instead.

[LP]

MSX, your argument is equally flawed at exceptions. In retail product, if you disable exceptions, then you risk of getting access violations. You can't predict all possible executions of your software. Also, if your user forgets to catch an exception, it is equal to crashing (the application behavior may be unpredicted after that). Actually, IMHO, if you would raise exception in list for index being out of bounds, I consider it as a improperly handled input, although it's a matter of opinion.

There is no point of start arguing whether the particular behavior of GetItem is good or bad (in YOUR opinion), in either case it is a VALID behavior. It handles all cases properly. Whether the user checks for nil or not is up to the user.

My point was that your application should handle as much input and behave correctly as possible, instead of just throwing error messages.

For instance: you set ScrollBar.Max to 150, then ScrollBar.Position to 100, then ScrollBar.Max to 50, the application should continue working instead of throwing error "Position is beyond max limit!".

[cairnswm]

Personally I think giving a default result is even better than raising any sort of error in a game. A game you want to be using as many clock cycles as possible for the game play and not coding error handling on every event.

So for a list, if an index is selected that doesn't exist return the first/last item instead.

This allows the developer just to develop their code and never worry about impossible results.

[{MSX}]

MSX, your argument is equally flawed at exceptions. In retail product, if you disable exceptions, then you risk of getting access violations.


Uhm i don't disable exceptions on retail producs.. They don't waste so much resources as to disable them, i think.



Also, if your user forgets to catch an exception, it is equal to crashing (the application behavior may be unpredicted after that).

It's not true that the behaviour may be unpredicted (well, it may of course, but for other reasons), becouse an exception is always catched by some handler. If you forget one, the more external one with take it. An exception can't be forgotten, by definition. Eventually with no handler, the whole application will crash, which is better IMHO than continuing with wrong behaviour.

More important, you get an immediate error (a more talking one than a generic AV error btw). With AV, the error can (and very often does) pop up in a completely different place:

// here i set myItem property of an object:
myObject.myItem := getItem(x); // this raises exception instantly, with the offending line number. No need to debug
...
// another unit, another thread, another caller far far away:
yourObject.myItem.getValue(); // access violation. But who set myItem to nil? Need to debug





There is no point of start arguing whether the particular behavior of GetItem is good or bad (in YOUR opinion), in either case it is a VALID behavior. It handles all cases properly. Whether the user checks for nil or not is up to the user.


But i stated that all was IMHO :)
They have a valid behaviour in the sense that they both work, but as i showed up here, they're quite different. I think the exception is better looking and better serving.



My point was that your application should handle as much input and behave correctly as possible, instead of just throwing error messages.


I think differently: i think an application should just take what can chew. If an application can't make sense of what it reads, using it could cause strange behaviour.



For instance: you set ScrollBar.Max to 150, then ScrollBar.Position to 100, then ScrollBar.Max to 50, the application should continue working instead of throwing error "Position is beyond max limit!".

In this example i agree with you, but in this case the application can make sense of the inexpected situation, it can eventually just scale the position.
Most depend also on the contract of each method or class. If it can be elastic without being wrong, i'm all with it.

But in many case there is something that you can't make sense of.
I agree with cairnswm that returning a default value (or doing the default behaviour) can be a good choiche, but you don't always have a default value.

I apologize if i look picky or worst :) I'm just showing my POV, i didn't intended to be crude.

[arthurprs]

At start of my game i was writing tons of error handling things, later because of the big time/line spending i stoped.

[Mirage]

In debug mode application should report all things that can be considired as errors. A reference to an invalid item in a list most likely means a logical error in your code and you should get to know about it ASAP. If you simply clamp invalid index to a valid range you will not have a chance to find the error. So I would GetItem be as follows:

function TPoints2px.GetItem&#40;Num&#58; Integer&#41;&#58; PPoint2px;
begin
Assert&#40;&#40;Num >= 0&#41;and&#40;Num <DataCount>= 0&#41;and&#40;Num < DataCount&#41; then Result&#58;= @Data&#91;Num&#93;
else Result&#58;= nil;
end;

In case of a "risky API call" fail it may be useful to write a line to log even in release version. It will help to find and fix problems remotely by end-user requests.

[Mirage]

Concerning exceptions - they have pros and cons:
+ all error-handling code in one (or more) places
+ an error occured in a low-lvel routine can be handled somewhere at a higher-level

- performance penalty even when no exception raised
- in a low-level library exceptions are inappropriate - some programmers (or projects) use them, some not, and the library should not to force use of exceptions.

So what to do? Fortunately Object Pascal has such a good thing as delegates (method pointers). The idea is to have a method-pointer variable within a library and when an error occurs, call the delegate. As an example I show how error handling is done in my stream class:

TError = class&#40;TMessage&#41;
ErrorMessage&#58; string;
constructor Create&#40;AErrorMessage&#58; string&#41;;
class function DefaultErrorHandler&#40;const Error&#58; TError&#41;&#58; Boolean;
end;

TErrorHandler = function&#40;const Error&#58; TError&#41;&#58; Boolean of object;

EStreamError = class&#40;TError&#41;
end;
EInvalidFormat = class&#40;TError&#41;
end;
EFileError = class&#40;TError&#41;
end;
...
var
ErrorHandler&#58; TErrorHandler;
...
function TFileStream.Read&#40;out Buffer; const Count&#58; Cardinal&#41;&#58; Cardinal;
begin
Result &#58;= 0;
if not Opened then if not ErrorHandler&#40;EStreamError.Create&#40;'File stream is not opened'&#41;&#41; then Exit;
BlockRead&#40;F, Buffer, Count, Result&#41;;
if Result > 0 then FPosition &#58;= FPosition + Result;
end;

The default ErrorHandler simply logs an error class and its text message. A library user can override this behaviour and make the delegate to raise exceptions (if he likes to use them) or even make a chain of error handlers at various levels of the application.

This solution doesn't have any performance penalties and has all advantages of exception-based approach.
Additionally, an error handler can fix the problem occured, return True and the routine which called the handler will continue its job. This can not be achieved with exceptions or result-code based approach.

What do you think about it, guys?

P.S.: It seems that a post can not have more than one code section?

[{MSX}]

That's a nice idea. Having a centralized choice on the behaviour is good. What i don't like is that you still have to code the "if error then log exit" idiom on each call (in the case you're returning false instead of raising an exception).

Btw about performance penalities for exception, are those so heavy? I don't know very well how they're lowered in the assembler, but it looks like it can be done in quite an efficent way.

[chronozphere]

wow... i see there is a nice discussion going on :D

There is another disadvantage of using exceptions (i guess nobody has mentioned it yet).
When your project consists of multiple DLL's, exceptions should be handled in the DLL that raised them, otherwise it may result in some nasty AV's. And a bug in the error-handling system is a true nightmare. :evil:

However, this is something i heard some time ago. If someone could explain WHAT EXACLTY causes the AV, please explain :razz:



A game you want to be using as many clock cycles as possible for the game play and not coding error handling on every event.


I think the clock-cycles are not the main isseu here... it's the time it takes to write the extra code + errorstrings, and the bloated ugly code, that makes extensive error-handling a bad thing.

@Mirage: That's a very nice way of handling errors. Very clever. :thumbup:



I think IMHO that the way of handling error of getItem (returning nil) is not good Smile
First, it doesn't solve the problem of having to check every function returning something:


I agree, but in a lot of cases, checking for NIL isn't neccesary.
The 'GetItem' routine is (in my experience) mostly used in the following context:

for i:=0 to GetItemCount -1 do
if GetItem(i).ProcessThisItem then
begin
GetItem(i).Process;
//etc etc.....
//......
end;


In this case, it is very unlikely that the index will go out of bounds, so checking for NIL would be foolish.



More important, you get an immediate error (a more talking one than a generic AV error btw). With AV, the error can (and very often does) pop up in a completely different place:


// here i set myItem property of an object:
myObject.myItem := getItem(x); // this raises exception instantly, with the offending line number. No need to debug
...
// another unit, another thread, another caller far far away:
yourObject.myItem.getValue(); // access violation. But who set myItem to nil? Need to debug



I think this scenario is unlikely to happen. When you get an item from a list, you usualy want to modify it directly or get a property-vallue or something. When you just want to store it in another more global variabele, you should check whether it's NIL or not.



I still tend to use the 'if error then exit' way, because your app doesn't get bloated. Some critical API calls do need error handling (e.g 3d Device Init, Flipping buffers or rendering). For some reason, i don't like to use exceptions (especialy in games).

I think using error-codes (dwords) instead of booleans would be a slight improvement. Then you need to make a list of error-types e.g:


E_SUCCES = 0;
E_FAIL = 1;
E_INVALID_PARAM = 2; //e.g a pointer parameter that shouldn't be NIL or points to wrong data
E_OUTOFBOUNDS = 3; //when an index is out of bounds
E_FILE_NOT_FOUND = 4; //when a file is missing
//etc etc....


This way you know something about the error and you can easily narrow down the ammount of places where the error can occur.

[{MSX}]

There is another disadvantage of using exceptions (i guess nobody has mentioned it yet).
When your project consists of multiple DLL's, exceptions should be handled in the DLL that raised them, otherwise it may result in some nasty AV's. And a bug in the error-handling system is a true nightmare. :evil:

However, this is something i heard some time ago. If someone could explain WHAT EXACLTY causes the AV, please explain :razz:


Well i never heard of this problem, probably becouse i don't have the habit of subdividing the programs into multiple dll.
It looks like it could be very possible, since exception handling is a bit triky when compiled (AFAIK).

If one uses multiple DLL it is probably better to make some test before using exception :)




I think this scenario is unlikely to happen. When you get an item from a list, you usualy want to modify it directly or get a property-vallue or something. When you just want to store it in another more global variabele, you should check whether it's NIL or not.


The getItem was just an example.. I agree that scrolling a list usually never return nil, but in many broader case the scenarios is much more common.
For example getting a property value, as you said, could involve dereferencing a class:

player.target.position; // AV if target is nil!




I still tend to use the 'if error then exit' way, because your app doesn't get bloated. Some critical API calls do need error handling (e.g 3d Device Init, Flipping buffers or rendering). For some reason, i don't like to use exceptions (especialy in games).


Sorry but weren't you who said that the 'if error then exit' pattern bloats the code? :P
If you meant instead bloating the executable binary code, i think a bloated executable is much much better than bloated code.
What are these reason why you don't like to use exceptions?
Maybe is it becouse you never tried them seriously? I say this becouse it is exacly the reason why i didn't used them before understanding how effective they are.



I think using error-codes (dwords) instead of booleans would be a slight improvement. Then you need to make a list of error-types e.g:

This way you know something about the error and you can easily narrow down the ammount of places where the error can occur.


This is exacly how error handling was done before the invention of exceptions.
Check some ancient C code or API (for example the socket API or the linux kernel) for some references :)

[LP]

Exceptions are a useful construct of object-oriented language, but although useful, they are generally misused.

Exceptions are best used internally, inside a single class or module. They should not be thrown off to the user for handling (this is why I said VCL is bad designed IMHO).

However, MSX, in the particular example you posted previously, it is a BAD USAGE of exceptions, considering that it is a bad practice in terms of Coupling and Cohesion (http://www.ugolandini.net/AccoppiamentoCoesioneE.html) (some recommended books are: this (http://www.amazon.com/Software-Design-Methodology-Principles-Architectural/dp/0750660759/ref=sr_1_1/105-4923317-5750830?ie=UTF8&s=books&qid=1189612346&sr=8-1) and this (http://www.amazon.com/Software-Engineering-International-Computer-Science/dp/0321313798/ref=pd_bbs_sr_1/105-4923317-5750830?ie=UTF8&s=books&qid=1189612442&sr=8-1)).

List out of bounds is an internal issue of your list component. By throwing exceptions outside, user will have to know about your exception and handle it, while it is part of your system. This lowers the Cohesion and increases Coupling.

The best practice would be to record the error and perhaps provide the user a way of checking it. However, user should not necessarily know about this and your component should work properly with any input the user may provide.

Returning "nil" although somehow bloats the code, as you mentioned (btw, the same argument we hear about Object-Oriented Programming in general, that it increases code lines as compared to Structural Programming), but:

1) It handles all user input without throwing errors.
2) All its output is valid (since the output value is a pointer, "nil" pointer is perfectly valid output).
3) It has a way of giving the end user information that the input was out of range.

According to Jef Raskin (http://en.wikipedia.org/wiki/Jef_Raskin):


The system should treat all user input as sacred.


You may like one way of handling errors or another. My advice was based on good practices of software design and it may not be perfect.

If you want to overuse exceptions and asserts to help the debugging and make your code shorter (hell, you can write all your code in one line, if number of lines is troubling you), it is your choice, but it may impact the quality of your products and the quality of development process, if there are more people working with you.

Finally, this is my favorite:


When you try to write the text of error message, please, stop and alter the interface so that a condition at which this error message is called, did not arise.

[chronozphere]

Sorry but weren't you who said that the 'if error then exit' pattern bloats the code? Razz
If you meant instead bloating the executable binary code, i thik a bloated executable is much much better than bloated code.
What are these reason why you don't like to use exceptions?
Maybe is it becouse you never tried them seriously? I say this becouse it is exacly the reason why i didn't used them before understanding how effective they are.


Ghehe.... no i said that if error then LOG exit makes the code bloated.... take a look at these examples.


//this is the 'If error then exit'
if not succeeded(ARiskyAPICall) then Exit;

//now here the error handling code is added
Result := ARiskyAPICall
if not Succeeded( Result ) then
begin
ReportError( Result, 'A risky api call failed', Module );
Exit;
end;

//now there is another case... e.g when a param is out of bounds
if (param <0> MaxParam) then
begin
Result := E_OUTOFBOUNDS;
ReportError( Result, 'The parameter was out of bounds', Module );
Exit;
end;



The last two cases look quite bloated while the first one is only a single line of code.
the second two need the 'begin end's' and those are actually bloating the code. ;)

If you want to use 'if error then exit', you should use it in the following context:

function MyRiskyFunction: boolean;
begin
Result := false;

if Error then Exit;
if AnotherError then Exit;

//only return true, when the whole function is executed without errors
Result := true;
end;


I will reconsider using exceptions. The big advantage is that you dont have to write all those errorchecking if's. If something goes wrong, every routine will cease execution until the exception is catched.