simple callback system for Py_FatalError

[bb0e0ed1-0da5-4aff-b6a2-55d779e4ece0]

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: ```python assignee = None closed_at = created_at = labels = ['interpreter-core', 'type-feature'] title = 'simple callback system for Py_FatalError' updated_at = user = 'https://bugs.python.org/mutkuk' ``` bugs.python.org fields: ```python activity = actor = 'vstinner' assignee = 'none' closed = True closed_date = closer = 'vstinner' components = ['Interpreter Core'] creation = 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'] ```

[bb0e0ed1-0da5-4aff-b6a2-55d779e4ece0]

Included implementation of a simple callback system for Py_FatalError.

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

[2ab6d433-92d2-47d3-b2d9-d90720887bfd]

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. \/

[bb0e0ed1-0da5-4aff-b6a2-55d779e4ece0]

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.

[2ab6d433-92d2-47d3-b2d9-d90720887bfd]

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.

[bb0e0ed1-0da5-4aff-b6a2-55d779e4ece0]

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.

[53b2d0ad-b56c-4a11-95a8-9fccf4dc9443]

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?

[53b2d0ad-b56c-4a11-95a8-9fccf4dc9443]

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. =\

[pitrou]

This sounds like a good idea but why the strange semantics of needing it to be defined before calling PyInitialize()?

[53b2d0ad-b56c-4a11-95a8-9fccf4dc9443]

I guess it seemed so unlikely that (C) extensions should be installing the callback that installation should be restricted pre-Py_Initialize(); the area completely controlled by the embedding app.

However, I have no strong attachment to that.

[amauryfa]

Here is a new patch; I lifted the pre-Py_Initialize() restriction, because it seems to me that a wxPython application, for example, could use it. A wxPython application is not embedded, but it already often redirects stdout and even installs a segfault handler.

I also made Py_SetFatalHook() return the previous hook; it could be useful to set a function temporarily, even if this is not thread safe.

[gpshead]

This makes sense, I'll add it to 3.3.

[vstinner]

fatalhook-2.patch: I don't understand the documentation. It says "Cause :cfunc:`Py_FatalError` to invoke the given function instead of printing to standard error and aborting out of the process.", but if the callback does nothing, the message+traceback is printed.

If the "fatal hook" is supposed to replace the function displaying the message, you should move the code displaying message+traceback in a subfunction and use it as the default hook function.

Pseudo code: ---------------

static fatal_error(const char *msg)
{
   printf("Fatal error: %s\n", msg);
   ... print traceback ...
}

static PyFatalHook fatalhook_func = fatal_error;

void
Py_FatalError(const char *msg)
{
    if (fatalhook_func != NULL)
       fatalhook_func(msg);
    ... windows debugger code ...
    abort();
}

---

NULL is a special hook value: don't print message+traceback, but exit (call abort()).

The hook can exit the process using something else than abort(). But if the hook doesn't exit, the default exit code is executed (call abort()), but the "Fatal error..."+traceback is not printed.

fatalhook_func != Py_FatalError

I think that this test is just overkill, or it should be moved to Py_SetFatalHook (e.g. set the hook to NULL if Py_FatalError is passed).

I also made Py_SetFatalHook() return the previous hook; it could be useful to set a function temporarily, even if this is not thread safe.

The previous hook can also be used to chain hooks. For example, if you would like to display the traceback but also do a special thing before exit, you can do something like: ------------------

void init()
{
   previous = Py_SetFatalHook(my_hook)
}

void my_hook(const char *msg)
{
  ... cleanup ...
  previous(msg);
}

---

About thread safety: because Py_FatalError() is called in the worst case (when something really bad happens), I think that it is better to not use something related to thread to avoid issues like deadlocks, and keep the code simple.

[amauryfa]

Sorry, the documentation in the patch is wrong. It should be: "Cause :cfunc:`Py_FatalError` to invoke the given function before printing to standard error and aborting out of the process."

I don't think it's worth making it more complex. If the application does not want the default behavior (which is already quite minimal anyway), it can always install a hook that never returns.

[vstinner]

Sorry, the documentation in the patch is wrong

Can you update your patch please?

[68239402-7baa-4248-82ae-f0bb13a55599]

The latest patch does not allow changing the default behaviour of aborting the process, which is for me a problem. I am both embedding and extending python using Boost.Python, but scripting is optional. In case python fails to initialize, I want to disable scripting but keep the program running.

Libraries shouldn't abort; they should report errors to the caller and let it figure out what to do.

[68239402-7baa-4248-82ae-f0bb13a55599]

While trying to come up with a patch, I'm starting to realize the number of modifications needed to make this work. Py_FatalError() is called all over the place assuming that it never returns. The scope of this feature is enormous.

I'm wondering what would happen if I threw an exception from there :)

[c9d81e0b-63aa-4f62-a22f-935b6ca67087]

Thinking about this again.. perhaps a better approach would be to force the embedder to define the symbol in their binary.

That is, libpython.x doesn't define Py_FatalError. The binary that links in libpython defines it.

(and a "me too" on Jonathan's comments.. for pg-python, abort() is not desirable.)

[gpshead]

i obviously didn't add this to 3.3, unassigning to reflect the attention it (isn't) currently getting. sorry!

[vstinner]

I understand that this issue has been fixed in bpo-36763 with the implementation of the PEP-587. A new API now allow to handle Python initialization error with a new PyStatus structure.

By the way, bpo-30560 was a similar issue.

Note: if I misunderstood the issue, please reopen it ;-)