Why leave the error handling to the caller?

[Richard Heathfield]

CBFalconer said:

Nonsense. It is entirely up to the programmer whether to call
xmalloc or malloc.

Yes, of course it is, and I didn't say otherwise. But not calling
xmalloc may, in this case, mean not calling a library function whose
functionality you really really need and whose source code you haven't
got and you only know it calls xmalloc because it shows up on the
backtrace when you try to work out why the thing keeps dumping core (if
indeed it does dump core - which it might not if the stupid thing is
calling exit rather than abort).

Frankly, I'm sick to the back teeth of Linux applications dropping dead
in the middle of a session. They are far more prone to it than Windows
applications, and I'm convinced it's because of this "do what I want or
I'll die in your face" attitude that prevails in the Linux community.

 

[Richard Heathfield]

Malcolm McLean said:

If we were releasing a medical app which was not debugged and in
consequence killed one person a year, the moral choice would be clear.

However we are releasing a game which spoils some kid's fun once a
year - remembering that for every time he can't kill the evil wizard
because it crashes when you cast magic mirror and invisibility at the
same time, he can't kill the evil wizard because mum has called him
for tea.

I'm not saying you shouldn't release games that have bugs in them that
you don't know about. I'm saying that you shouldn't release games that
have bugs in them that you /do/ know about, or are deliberately not
checking for.

If we don't release when the marketing people want, it could
easily take down the company.

If you deliberately release shoddy products, your company has no
long-term future anyway.

 

[Richard Heathfield]

Malcolm McLean said:

There is no pleasing some people.
You wanted it to compile, so now it compiles.

Excuse me? I didn't even mention the fact that it didn't compile. What I
said was that "I looked at your code, and found it lacking in any
significant evidence that you are a regular clc subscriber."

The second lot of code did nothing to change my mind.

Nevertheless, I can use the code you posted to illustrate my point.
Let's just pretend that makestrings is well-written, shall we? And if
it fails, it returns NULL. Your loop for handling makestrings assumes
it succeeded, which is unwise, but let's fix that:

Your code:

str = makestrings(argv[1]);
for(i=0;i<str->N;i++)
printf("***%s***\n", str->str);

would be better written as:

str = makestrings(argv[1]);
if(str != NULL)
{
for(i = 0; i < str->N; i++)
{
printf("***%s***\n", str->str);
}
}
else
{
char *p = argv[1];
char *q;
for(q = strchr(p, ','); q != NULL; q = strchr(p, ','))
{
printf("***%.*s***\n", q - p, p);
p = q + 1;
}
printf("***%s***\n", p);
}

And thus we have a recovery strategy which achieves the correct program
output even in the face of a malloc failure. And that's why library
routines shouldn't bomb out.

QED.

 

[Richard Harter]

Malcolm McLean said:

[snip]
If we don't release when the marketing people want, it could
easily take down the company.

If you deliberately release shoddy products, your company has no
long-term future anyway.

And your evidence for this is?

 

[Dave Vandervies]

[snip xmalloc code that terminates program on allocation failure]

That's as may be, but I do hope that that code didn't find its way into
the SHTTP parts of, e.g., Firefox.

I expect not; I believe OpenSSH uses OpenSSL for its encryption, and
other code that needs encryption would be more likely to build on top
of OpenSSL (with, one hopes, more reusable-library-appropriate error
handling) than OpenSSH.

Crash&dump is all very well for a
small command line tool, but if an application can hold any serious
amount of data in memory, it's an evil thing to do to your users.

You're thinking too small. Firefox likes to bring the whole windowing
system down on resource allocation failures. (Though I haven't used
recent versions for long-running sessions, so there's a chance they
might have finally fixed that.)


dave

 

[CBFalconer]

Malcolm McLean said:
[snip]

If we don't release when the marketing people want, it could
easily take down the company.

If you deliberately release shoddy products, your company has
no long-term future anyway.

And your evidence for this is?

Microsoft - ta-da

 

[Johan Bengtsson]

Malcolm McLean said:

I'm not saying you shouldn't release games that have bugs in them that
you don't know about. I'm saying that you shouldn't release games that
have bugs in them that you /do/ know about, or are deliberately not
checking for.


If you deliberately release shoddy products, your company has no
long-term future anyway.

If the products are not released reasonably close to on time the company
has no long term future either.

I am mostly writing educational program that contains almost no really
valuable information only in memory at any time. malloc() or die Is the
technique I use most frequently (not always however - loading a picture
for example have separate checks ignoring the display of that picture
rather than killing the application). I have two cases for allocating
most of my memory: 1. during the loading of a part of the course, die
here causes some annoying restart of the application but no data loss.
2. logging of values (in the process control simulations values are
periodically logged to be displayed on a graph), die here is not
preferable (but as it is implemented now it does), a preferable method
would be to remove the oldest logged values from memory. However doing
that would have delayed the delivery and rarely happens anyway. I have
not yet seen or heard of any customer thinking this is a too bad
behavior. It *will* be fixed, but there are higher priorities.

Misunderstand me correctly: I do agree that the decision should not be
placed in a library function. In have in my private library functions
done like this:
- Created a general "memory allocation has failed, what to do next?"
function with the (currently unused but available) possibility to
register callback functions for freeing memory that is not very
important (logger values or undo buffers are example of such memory).
- Created a xmalloc() and friends (with another name but that's not a
point) that calls the first functon on failure. (I also have a wrapper
for the standard malloc() and fiends, but that is because I need to
handle the case where a plugin is compiled with another compiler and
thereby another C runtime library version). I also use those for
checking for unfreed memory blocks at program termination in the debug
version of my program.
- Most of the rest of the functions in that library have two versions,
one using malloc() and one using xmalloc(). This might be a waste of
code at first glance, but since I can concentrate the error handling in
the most places where I do allocate memory it actually saves code (most
memory allocation is during load - no important data in memory anyway).

As mentioned, the only other sensible solution in a library would be to
pass an error handling function to all functions capable of memory
allocation. You could then define two such functions inside the library
- one that always displays a message and dies and one that fails the
allocation (like standard malloc()). The programmer might then pass
another function doing anything else, including asking the user or
freeing some other memory and then retrying.

 

[Johan Bengtsson]

Malcolm McLean said:

There is no pleasing some people.
You wanted it to compile, so now it compiles.

Excuse me? I didn't even mention the fact that it didn't compile. What I
said was that "I looked at your code, and found it lacking in any
significant evidence that you are a regular clc subscriber."

The second lot of code did nothing to change my mind.

Nevertheless, I can use the code you posted to illustrate my point.
Let's just pretend that makestrings is well-written, shall we? And if
it fails, it returns NULL. Your loop for handling makestrings assumes
it succeeded, which is unwise, but let's fix that:

Your code:

str = makestrings(argv[1]);
for(i=0;i<str->N;i++)
printf("***%s***\n", str->str);

would be better written as:

str = makestrings(argv[1]);
if(str != NULL)
{
for(i = 0; i < str->N; i++)
{
printf("***%s***\n", str->str);
}
}
else
{
char *p = argv[1];
char *q;
for(q = strchr(p, ','); q != NULL; q = strchr(p, ','))
{
printf("***%.*s***\n", q - p, p);
p = q + 1;
}
printf("***%s***\n", p);
}

And thus we have a recovery strategy which achieves the correct program
output even in the face of a malloc failure. And that's why library
routines shouldn't bomb out.

QED.

Ummm, am I missing something here?
I do agree that the library functions should not make the decision and
kill the program, and (depending on the program) returning NULL from
makestrings would be the the best behaivour.
I do however not really agree to this solution of error handling. If the
intended behavior of the program is to simply output all the strings
with no further processing there would never be any reason for storing
them (and thereby not having any trouble to begin with) and whatever is
in the error recovery part of the program would better be the *entire*
program. I assume the purpose of allocating the strings in some
structure to begin with is that there really is some further processing
to be done before outputting them (for example sorting or whatever).
That would make the error handling function output data that would be
wrong. Isn't it (in most cases) then better to not get any data than to
get the wrong data?

The answer to that does of course depend on the purpose of the
application - and can only be answered by the ones ordering the program
to be written in the first place and the intended customers.
Assume, for example, that the strings should be sorted: I would rather
have the program fail completely than output the strings unsorted. If
the purpose is to just output the strings in order of appearance - then
it would be very bad programming to allocate memory for them anyway
since that isn't necessary to complete the task.

 

[Mark McIntyre]

Microsoft - ta-da

Cue Man-falling-off-skyscraper joke... ok so far...
--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan

 

[Richard Heathfield]

Richard Harter said:

Malcolm McLean said:

[snip]
If we don't release when the marketing people want, it could
easily take down the company.

If you deliberately release shoddy products, your company has no
long-term future anyway.

And your evidence for this is?

The obvious example is Ratner (the jeweller, or rather, the
ex-jeweller). And Microsoft is the obvious counter-example (so far,
anyway).

In general, it's a mistake to think customers are stupid.

 

[Richard Heathfield]

Johan Bengtsson said:

Richard Heathfield wrote:


Ummm, am I missing something here?
No.

I do agree that the library functions should not make the decision and
kill the program, and (depending on the program) returning NULL from
makestrings would be the the best behaivour.

That's the whole thing. So you're not missing anything.

I do however not really agree to this solution of error handling. If
the intended behavior of the program is to simply output all the
strings with no further processing there would never be any reason for
storing them (and thereby not having any trouble to begin with) and
whatever is in the error recovery part of the program would better be
the *entire* program.

But consider that there might be a real benefit to be had from the
dynamic allocation solution which couldn't be got from the static
solution (e.g. raw speed, perhaps); nevertheless, the static solution
would be "good enough" (albeit slower, perhaps). The status bar might
read something like "want 38GB for WhizSort, failed to get 38GB, can't
do WhizSort, switching to TrudgeSort..."

 

[Eric Sosman]

Johan Bengtsson wrote On 06/21/07 13:04,:

[...]
I am mostly writing educational program that contains almost no really
valuable information [...]

Yes, we've all taken a few courses of that kind.

 

[Malcolm McLean]

Johan Bengtsson said:


That's the whole thing. So you're not missing anything.

No, the specification was to produce a function that filled out the
structure giving a list of strings, from an input consisting of a line of
strings separated by commas.
main() was a test driver to ensure that the function worked. However it
didn't test for memory allocation failure. You can get compilers which will
do dicky malloc()s that fail unpredictably but frequently, but Microsoft's
freebie isn't one of them. Thus the error-handling code was in fact
untested.

Richard Heathfield pointed out a bug in the test driver. So really we need
another test suite to test the test code.

But consider that there might be a real benefit to be had from the
dynamic allocation solution which couldn't be got from the static
solution (e.g. raw speed, perhaps); nevertheless, the static solution
would be "good enough" (albeit slower, perhaps). The status bar might
read something like "want 38GB for WhizSort, failed to get 38GB, can't
do WhizSort, switching to TrudgeSort..."

If WhizSort does need 38GB then that's fair enough. However if it is 4KB,
and the OS won't give it to you, it is unlikely that Trudge Sort will be any
more successful. The sort will work if it takes a fixed buffer, but the OS
might hang when you call printf() to output the results.

 

[Richard Heathfield]

Malcolm McLean said:

No, the specification was to produce a function that filled out the
structure giving a list of strings, from an input consisting of a line
of strings separated by commas.

Separate the concept of a list of strings from the concept of input
parsing.

Now separate the concept of a list from the concept of a string.

Now the error handling is becoming manageable (you will recall that you
originally wrote that code to demonstrate that the error handling was a
PITN to write), and the program's complexity is being reduced as more
and more work is farmed out to re-usable library modules.

What you end up with is rather elegant (if done properly), and eminently
re-usable.

 

[Richard Bos]

You're thinking too small. Firefox likes to bring the whole windowing
system down on resource allocation failures. (Though I haven't used
recent versions for long-running sessions, so there's a chance they
might have finally fixed that.)

It does? I've never seen that happen on M$'doze. Perhaps X is buggy

Richard

 

[Malcolm McLean]

Malcolm McLean said:



Separate the concept of a list of strings from the concept of input
parsing.

Now separate the concept of a list from the concept of a string.

Now the error handling is becoming manageable (you will recall that you
originally wrote that code to demonstrate that the error handling was a
PITN to write), and the program's complexity is being reduced as more
and more work is farmed out to re-usable library modules.

What you end up with is rather elegant (if done properly), and eminently
re-usable.

The problem is you start introducing dependencies.
The example is a little bit artificial because we haven't said what we are
using our list of strings for.
However a programming interface

STRINGS *list = makestrings("One,two,three,four");
if(!list)
/* that old problem again - a 2GB computer is more likely to break
than to execute this */
exit(EXIT_FAILURE);
/* use strings */
killstrings(list);

is perfectly reasonable; you might want to pass a series of strings in one
go and one parameter rather than mess about with add_string functions.
Certainly an object constructor very often needs to take a list.

The question is the internals. High-level languages achieve their neatness
by using dynamic arrays. We can do that in C, but only by seriously messing
about with the language.

/*
add an item to a dynamic list.
Params: ptr - pointer allocated with malloc(), can be null
N - number of items pointer currently holds
item - item to add to list
sz - element size
Returns: pointer to new array on success, 0 on fail.
*/
void *dlist_add(void *ptr, int N, void *item, size_t sz)
{
unsigned char *temp;

temp = realloc(ptr, (N + 1) * sz);
if(!temp)
return 0;
memcpy(temp + N * sz, item, sz);
return temp;
}

Now we've got the equivalent of an STL-style vector, but it is horrid,
horrid. No one is going to use that.
Then if we put dlist_add into a separate file, suddenly we got a dependency,
and the old problem of you want a list of strings and that requires pulling
in a whole hierarchy of files.

 

[Malcolm McLean]

In general, it's a mistake to think customers are stupid.

I'm afraid the big console companies, but not the games programmers
themselves, have complete contempt for their customers.

We were allowed to show two error messages. "Drive door not open" and
"Disk dirty". So if the program encounters an internal error, such as
running out of memory, the customer has to take the disk out, polish it, and
restart the game, and that fixes the problem. Says it all.

 

[Richard Heathfield]

Malcolm McLean said:

The problem is you start introducing dependencies.

The only way to avoid dependencies is to have everything defined in
main, including the standard library. "Dependencies" is just an Eeyore
way of saying "don't have to write it again because we can use the one
we wrote last time".

The example is a little bit artificial because we haven't said what we
are using our list of strings for.
However a programming interface

STRINGS *list = makestrings("One,two,three,four");
if(!list)
/* that old problem again - a 2GB computer is more likely to break
than to execute this */

Wrong. A 2GB computer will typically have many more demands placed upon
it than will a 640KB computer, and every camel - no matter how strong
its back - has a last straw.

/*
add an item to a dynamic list.
Params: ptr - pointer allocated with malloc(), can be null
N - number of items pointer currently holds
item - item to add to list
sz - element size
Returns: pointer to new array on success, 0 on fail.
*/
void *dlist_add(void *ptr, int N, void *item, size_t sz)
{
unsigned char *temp;

temp = realloc(ptr, (N + 1) * sz);
if(!temp)
return 0;
memcpy(temp + N * sz, item, sz);
return temp;
}

Now we've got the equivalent of an STL-style vector, but it is horrid,
horrid. No one is going to use that.

Agreed. I'm certainly not going to use it. But one is not /required/ to
write horrid, horrid code, even in C.

Then if we put dlist_add into a separate file, suddenly we got a
dependency,

....and who'd want to depend on horrid, horrid code, right?

But I don't have any problem depending on /good/ code. Do you?

and the old problem of you want a list of strings and that
requires pulling in a whole hierarchy of files.

No, just one - it's called a "library". Okay, the header is also needed,
so make that two.

 

[Malcolm McLean]

Malcolm McLean said:
Wrong. A 2GB computer will typically have many more demands placed
upon it than will a 640KB computer, and every camel - no matter how
strong its back - has a last straw.

What you are saying is that you have a small pension from the British
government. What is the chance of the government going bankrupt as you
withdraw your pension?
Governments can go bankrupt, but it causes major social disruption, and so
voters normally don't tolerate more than one or two defaults until they
either put more money into the kitty or reduce obligations.

But I don't have any problem depending on /good/ code. Do you?

Yes. I am currently writing BabyX purely to avoid depending on Motif. Motif
is a far better toolkit than I could write, but I don't have it installed on
my machine. I can get it, but then that will mean that everyone who wants to
play about with my program will have to do the same. So I took the decision
to depend just on Xlib, hence BabyX.

 

[Richard Heathfield]

Malcolm McLean said:

What you are saying is that you have a small pension from the British
government. What is the chance of the government going bankrupt as you
withdraw your pension?

Lower than the probability of a 2GB machine running low on RAM.
Nevertheless, if the Government writes me a cheque, I'll check that it
clears before trying to spend the money.

<snip>