[GRASS-dev] Vect_*_fatal_error()

[Martin Landa]

Hi,

2011/11/12 Glynn Clements <glynn at gclements.plus.com>:
> Even if the exit()-on-fatal-error issue magically disappeared, the
> memory management issues (i.e. don't bother tracking allocations which
> don't matter for a typical module) won't. Nor will the fact that
> "try ... except" won't catch a segfault.
>
> grass.lib.* exists for "module-style" scripts, not for persistent
> applications.

that's right, but we just shouldn't forbid to use GRASS libraries for
such applications (for which the GRASS libraries were not designed).
As we all know they were designed for the GRASS modules. I just think
that it's very very bad political decision to *forbid* the programmer
to changes behaviour of G_fatal_error(), or better to say to avoid
calling exit() if he/she really wants to go on even if there was a
fatal error. It's not over business, we just say, that the GRASS
libraries has been designed for that cases, so you will remain in
undefined stage and you should force the user to quit the whole
application or so. The reason why I raised this issue again I think
that giving no chance to change behaviour of G_fatal_error() is just
ignoring and making life very difficult to non-GRASS or currently
wxGUI programmers. I really think that we should give the programmer
possibility to override G_fatal_error() by something which is not
calling exit() if he/she really needed and let him/her know that the
GRASS libraries remains in undefined stage. It's not over business,
it's up to the programmer.

>> Well due to design, anyway very user
>> friendly. I am not a fan of G_set_fatal_error() solution, on the other
>> hand I don't see any reason why to *forbid* so strictly the
>> programmers which are using GRASS libraries to avoid calling system
>> exit on G_fatal_error(). The default behaviour will remain! We just
>> let other programmers (with a big warning) to use GRASS libraries for
>> the purpose which they were not originally designed nothing more.
>
> Allowing G_fatal_error() to return isn't going to stop wxGUI, QGIS,
> etc terminating unexpectedly. It's just going to make them terminate
> via a segfault rather than via exit().

Not always in every case. What about Rast_open_old() which calls also
G_fatal_error(). If you try in that application to open raster map
(which fails) the application just crashes without no message. Good;-)
You have no possibility to show message what failed why it failed and
eg. force the user to quit the application if needed.

> If a function calls G_fatal_error(), it isn't expecting it to return.
> If it does return, the usual result will be that the caller attempts
> to use invalid data; a crash isn't likely to be far off.
>
> A typical example of G_fatal_error() usage is:
>
>        FILE *fp = fopen(...);
>        if (!fp)
>            G_fatal_error(...);
>        fread(..., fp);
>
> Allowing G_fatal_error() to return will simply replace the exit() with
> a segfault. Why would this be an improvement?

The improvement is that you can show at least some error message and
to quit application with knowing user that the fatal error appeared,
where appeared, etc. In the most cases it will be bug in the
application, at least the application will just not crash without any
message.

> If you want to (attempt to) recover from fatal errors, you have to
> escape from G_fatal_error() via a longjmp(). Either that, or you have
> to re-write every single use of G_fatal_error() (~3800 of them) to
> accomodate the possibility of it returning.

I would prefer to modify G_fatal_error() rather then use longjmp()
which is just basically a workaround. Moreover the application can
still fail in the same way. It's not a safe solution.

> Except that the latter isn't really an acceptable solution, as it
> pushes the burden of error handling on to everything else. You might
> consider making your job easier justifies making everyone else's
> harder, but don't expect everyone else to agree.
>
> So long as I have commit privileges, G_fatal_error() is going to
> retain (and honour) the "noreturn" attribute.

WHY SO? That are very very strong words to me! I thought that the
GRASS project is still community based and nobody has a veto
privilege. Probably I am wrong. Please consider also my reasoning.

> Aside: In 7.0, I changed a number of library functions which had
> previously returned a status code to call G_fatal_error() instead.
> This meant eliminating the status-checking from the callers, as the
> return type changed from "int" to "void".

Right, as for Rast_open_old(), the result is that the application for
which the GRASS library hasn't been designed eg. the wxGUI just
crashes without no message, eg. if raster map doesn't exits. Is it
really fatal? Even I don't consider as fatal error if you open raster
and it fails. For GRASS modules it just terminate the process, for
long running application it means complete crash. This is bad. It just
avoids need to call before G_find_raster() in normal GRASS modules and
make it harder for long running application.

> Which lead me to discover that, for modules, actually bothering to
> check the status code tended to be the exception rather than the rule.
> IOW, even with most functions using G_fatal_error() to eliminate the
> need to check status codes, the burden of dealing with the few which
> didn't was still too much.

[...]

Martin

-- 
Martin Landa <landa.martin gmail.com> * http://geo.fsv.cvut.cz/~landa