Repository navigation
simple callback system for Py_FatalError #41948
Description
| BPO | 1195571 |
|---|---|
| Nosy | @birkenfeld, @gpshead, @amauryfa, @pitrou, @vstinner, @phmc |
| Files |
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']
Activity
mutkuk commentedon May 4, 2005
Included implementation of a simple callback system for
Py_FatalError.
Everything is done in "pythonrun.c" and "pydebug.h"
jwpye commentedon May 5, 2005
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}
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
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
150a161,162
1507a1520,1523
mutkuk commentedon May 5, 2005
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:
Declaration of register func would better be in pydebug.h
near Py_FatalError.
Last one to call register func owns the hook, this is
just expected case so IMHO remove NULL check from register func.
jwpye commentedon May 5, 2005
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... =\
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 commentedon May 5, 2005
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.
Not worth to remove it IMHO just feels OK
Got it, agreed.
[-]simple callback system for Py_FatalError [/-][+]simple callback system for Py_FatalError[/+][-]simple callback system for Py_FatalError [/-][+]simple callback system for Py_FatalError[/+]jwp commentedon Feb 16, 2009
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 commentedon Jul 3, 2009
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 commentedon Jul 29, 2010
Amaury, any interest in getting this committed for 3.2?
jwp commentedon Aug 2, 2010
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. =\
4 remaining items
vstinner commentedon Jun 8, 2011
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 PyFatalHook fatalhook_func = fatal_error;
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:
------------------
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 commentedon Jun 9, 2011
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 commentedon Jul 8, 2011
Can you update your patch please?
jonathanmcdougall commentedon Dec 22, 2012
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.
jonathanmcdougall commentedon Dec 23, 2012
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 :)
JamesPye commentedon Apr 24, 2013
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 commentedon Nov 23, 2013
i obviously didn't add this to 3.3, unassigning to reflect the attention it (isn't) currently getting. sorry!
vstinner commentedon May 27, 2019
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 ;-)