simple callback system for Py_FatalError

[mutkuk]

BPO	1195571
Nosy	@birkenfeld, @gpshead, @amauryfa, @pitrou, @vstinner, @phmc
Files	new_fatalhook.diff: Py_FatalError hook function fatalhook.patch fatalhook-2.patch: new patch, with "versionadded" tags

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-05-27.15:15:35.676>
created_at = <Date 2005-05-04.22:57:11.000>
labels = ['interpreter-core', 'type-feature']
title = 'simple callback system for Py_FatalError'
updated_at = <Date 2019-05-27.15:15:35.675>
user = 'https://bugs.python.org/mutkuk'

bugs.python.org fields:

activity = <Date 2019-05-27.15:15:35.675>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2019-05-27.15:15:35.676>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2005-05-04.22:57:11.000>
creator = 'mutkuk'
dependencies = []
files = ['6642', '14440', '19098']
hgrepos = []
issue_num = 1195571
keywords = ['patch', 'needs review']
message_count = 21.0
messages = ['48294', '48295', '48296', '48297', '48298', '82266', '90062', '111967', '112494', '112495', '112498', '117839', '137922', '137926', '137942', '140014', '177950', '177965', '187679', '204080', '343642']
nosy_count = 12.0
nosy_names = ['georg.brandl', 'gregory.p.smith', 'amaury.forgeotdarc', 'pitrou', 'vstinner', 'mutkuk', 'jwp', 'Tom.Whittock', 'pconnell', 'jonathanmcdougall', 'isoschiz', 'James.Pye']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1195571'
versions = ['Python 3.3']

[mutkuk]

Included implementation of a simple callback system for
Py_FatalError.

Everything is done in "pythonrun.c" and "pydebug.h"

[jwpye]

Logged In: YES
user_id=1044177

Again, I think this should be explicitly exclusive to an
embedding application, as I cannot imagine a situation where
an extension module should even have access to this facility.

I have done rather limited tests. My application doesn't
seem to like Python 2.5alpha, so I'm going to test the patch
against 2.4.

Sorry to butt into your patch, mutkuk, but here's my
recommendation with docs(ugh, cant attach files):

Index: Doc/api/init.tex
===================================================================
RCS file: /cvsroot/python/python/dist/src/Doc/api/init.tex,v
retrieving revision 1.23
diff -r1.23 init.tex
39a40,53

\begin{cfuncdesc}{void}{Py_InitializeFatalHook}{PyFatalHook fp}

Force \cfunction{Py_FatalError()} to invoke the given
function
instead of printing to standard error and aborting out
of the
process. This is intended for use in embedded
applications that
want to define their own route for handling fatal
errors. Giving
the application the ability to log the error to a special
destination, and to do any appropriate cleanups before
exiting.

If used, this \emph{must} be called prior to
\cfunction{Py_Initialize()}. Otherwise,
\cfunction{Py_Initialize()}
will set the hook to the default, and future attempts to
\cfunction{Py_InitializeFatalHook()} will be ignored.
\end{cfuncdesc}

Index: Include/pythonrun.h
===================================================================
RCS file: /cvsroot/python/python/dist/src/Include/pythonrun.h,v
retrieving revision 2.65
diff -r2.65 pythonrun.h
24a25,27

typedef void (*PyFatalHook)(const char *);
PyAPI_FUNC(void) Py_InitializeFatalHook(PyFatalHook);

Index: Python/pythonrun.c
===================================================================
RCS file: /cvsroot/python/python/dist/src/Python/pythonrun.c,v
retrieving revision 2.213
diff -r2.213 pythonrun.c
99a100,109

static PyFatalHook _fatalhook = NULL;

void
Py_InitializeFatalHook(PyFatalHook f)
{
/* Only set it if it is uninitialized. */
if (_fatalhook == NULL)
_fatalhook = f;
}

150a161,162

Py_InitializeFatalHook(Py_FatalError);

1507a1520,1523

if (_fatalhook != Py_FatalError && _fatalhook != NULL)
_fatalhook(msg);
/* If the hook fell through, finish the process anyways. */

[mutkuk]

Logged In: YES
user_id=1272056

Extension case:
Extension developers may mess up too, why leave them orphaned?

Doc case:
UGGGHHH

Your patch is just slicker, why dont I delete this patch and
you submit. What do you say?

I have just 2 suggestions:

1. Declaration of register func would better be in pydebug.h
   near Py_FatalError.

2. Last one to call register func owns the hook, this is
   just expected case so IMHO remove NULL check from register func.

[jwpye]

Logged In: YES
user_id=1044177

Extension case:
Really, if an extension requires special cleanup, chances
are the application will take notice of it and probably
handle that cleanup itself. If extension developers thought
that it was okay to overload the hook, then they might make
the mistake of overloading the hook in an embedded
application where fatal cleanup was a necessity. If a stack
of hooks were provided, who or what determines which hook
should cause the exit? I don't actually have control over
what PostgreSQL does with its FATAL ereport, so if it hit my
hook first, the application will go bye bye, without
touching the others. Also, it might be possible for
extensions to tap into some signal to allow cleanup. For
instance abort(3) sends the process a SIGABRT. All in all, I
still think it's bad mojo.

1: I see your reasoning, but I disagree as when the
developer pokes around the headers it will be more useful
for it, the typedef, to be around the *only* call that the
typedef is used for. Thus showing the complete expectation
for the function's argument, without having to find it in
another file. Matter of fact, I think the typedef isn't
really all that useful except to declare the static function
pointer in pythonrun.c, so I'd rather do without it... =\

2. Oh, yeah the NULL check is there just in case someone
   gets smart and tries to start running Python calls before
   Python is initialized(ala Py_InitializeEx()). Besides that
   it, Py_FatalError should only be called once during the
   process lifetime, so the extra condition isn't really much
   of a performance issue.

I'll e-mail you the patch.

[mutkuk]

Logged In: YES
user_id=1272056

Uploaded new patch unchanged.

Extension case:
This func categorizes with PyThreadSatate_*. For debugger
implementors(generally implmented as extension) it is
invaluable. Indeed that's the reason I needed it in the
first place.

1: I see your reasoning, but I disagree as when the
developer pokes around the headers it will be more useful
for it, the typedef, to be around the *only* call that the....

Not worth to remove it IMHO just feels OK

2. Oh, yeah the NULL check is there just in case someone
   gets smart and tries to start running Python calls before
   Python is initialized(ala Py_InitializeEx()). Besides that
   it, Py_FatalError should only be called once during the
   process lifetime, so the extra condition isn't really much
   of a performance issue.

Got it, agreed.

[jwp]

I had actually forgotten that this was still open. Anything I can do to
help speed this along?

In case nobody remembers, the purpose of this was to provide a facility
to embedded applications that allows for a more graceful shutdown in
fatal error situations. My original use-case was for a PostgreSQL PL
extension where an abort() is quite undesirable as it would take down
the entire cluster(yeah, all the postmaster processes).

IIRC, the only thoughts "against" it were rather for trying to remove
FatalErrors entirely.

hepl?

[amauryfa]

Here is a refreshed patch against trunk.
Since the previous one did not provide any context, I hope I copied the
lines in the right locations.

[birkenfeld]

Amaury, any interest in getting this committed for 3.2?

[jwp]

Would it be possible to require the embedding application to define the Py_FatalError symbol?

Admittedly, it would be nice to not have the callback installation code. =\