-
-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
simple callback system for Py_FatalError #41948
Comments
Included implementation of a simple callback system for Everything is done in "pythonrun.c" and "pydebug.h" |
Logged In: YES Again, I think this should be explicitly exclusive to an I have done rather limited tests. My application doesn't Sorry to butt into your patch, mutkuk, but here's my Index: Doc/api/init.tex \begin{cfuncdesc}{void}{Py_InitializeFatalHook}{PyFatalHook fp}
Index: Include/pythonrun.h
Index: Python/pythonrun.c
150a161,162
1507a1520,1523
|
Logged In: YES Extension case: Doc case: Your patch is just slicker, why dont I delete this patch and I have just 2 suggestions:
|
Logged In: YES Extension case: 1: I see your reasoning, but I disagree as when the
I'll e-mail you the patch. |
Logged In: YES Uploaded new patch unchanged. Extension case:
Not worth to remove it IMHO just feels OK
Got it, agreed. |
I had actually forgotten that this was still open. Anything I can do to In case nobody remembers, the purpose of this was to provide a facility IIRC, the only thoughts "against" it were rather for trying to remove hepl? |
Here is a refreshed patch against trunk. |
Amaury, any interest in getting this committed for 3.2? |
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. =\ |
This sounds like a good idea but why the strange semantics of needing it to be defined before calling PyInitialize()? |
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. |
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. 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. |
This makes sense, I'll add it to 3.3. |
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.
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).
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. |
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. |
Can you update your patch please? |
The latest patch does not allow changing the default behaviour of Libraries shouldn't abort; they should report errors to the caller and |
While trying to come up with a patch, I'm starting to realize the number I'm wondering what would happen if I threw an exception from there :) |
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.) |
i obviously didn't add this to 3.3, unassigning to reflect the attention it (isn't) currently getting. sorry! |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: