Hi,
I recently modified Python 3.8 to start logging I/O error in io destructors when using the development mode (-X dev) or when Python is built in debug mode. This change allows to debug bpo-18748 very strange crash in PyThread_exit_thread(), when a thread closes an arbitrary file descriptor by mistake.
The problem is that exceptions raised in a destructor cannot be passed to the current function call: the exception is logged into sys.stderr with a message like "Exception ignored in: ...". It's easy to miss such message in stderr.
Thomas Grainger opened 36829 to propose to add -X abortunraisable command line option: abort immediately the current process (by calling Py_FatalError()) at the first PyErr_WriteUnraisable() call. Thomas had like a way to easily detect these in CI. I'm not satisfied by his proposal, since it only gives little control to the user on how "unraisable exceptions" are handled: the process dies, that's all.
https://bugs.python.org/issue36829
I proposed a different approach: add a new sys.unraisablehook hook which is called to handle an "unraisable exception". To handle them differently, replace the hook. For example, I wrote a custom hook to log these exceptions into a file (the output on the Python test suite is interesting!). It also becomes trivial to reimplement Thomas's idea (kill the process):
import signal def hook(unraisable): signal.raise_signal(signal.SIGABRT) sys.unraisablehook = hook
I plan to merge my implementation soon, are you fine with that?
https://github.com/python/cpython/pull/13187
--
The first implementation of my API used sys.unraisablehook(exc_type, exc_value, exc_tb, obj). The problem is that Serhiy Storchaka asked me to add a new error message field which breaks the API: the API is not future-proof.
I modified my API to create an object to pack arguments. The new API becomes sys.unraisablehook(unraisable) where unraisable has 4 fields: exc_type, exc_value, exc_tb, obj. This API is now future-proof: adding a new field will not break existing custom hooks!
By the way, I like this idea of adding an optional error message, and I plan to implement it once sys.unraisablehook is merged ;-) I already implemented it previously, but I reverted my changes to make the PR simpler to review.
Extract of the documentation:
""" Add new :func:`sys.unraisablehook` function which can be overriden to control how "unraisable exceptions" are handled. It is called when an exception has occurred but there is no way for Python to handle it. For example, when a destructor raises an exception or during garbage collection (:func:`gc.collect`). """
My implementation has a limitation: if PyErr_WriteUnraisable() is called after the Python finalization cleared sys attributes (almost the last function call of Py_Finalize), the default hook is called instead of the custom hook. In this case, sys.stderr is None and so the default hook does nothing.
These late calls to PyErr_WriteUnraisable() cannot be catched with my approached, whereas Thomas Grainger's command line option "-X abortunraisable" allows that.
My concern with Thomas's approach is that if the problem is killed with SIGABRT by such late PyErr_WriteUnraisable(), a low-level debugger like gdb is needed to investigate the crash, since Python is already finalized and so cannot be used to investigate.
I prefer to allow arbitrary hook with the limitation, rather than always kill the process with SIGABRT at the first PyErr_WriteUnraisable() and require to use a low-level debugger.
Victor
On Wed, May 15, 2019 at 6:25 PM Victor Stinner vstinner@redhat.com wrote:
I proposed a different approach: add a new sys.unraisablehook hook which is called to handle an "unraisable exception". To handle them differently, replace the hook. For example, I wrote a custom hook to log these exceptions into a file (the output on the Python test suite is interesting!). It also becomes trivial to reimplement Thomas's idea (kill the process):
What happens if the hook raises an exception?
-n
Le jeu. 16 mai 2019 à 04:44, Nathaniel Smith njs@pobox.com a écrit :
What happens if the hook raises an exception?
Aha, thanks for asking the question!
If there is a custom hook and the hook fails, the default hook logs the exception of the custom hook.
Technically, even if the default hook fails, the default hook is called again to handle its own exception :-)
The fallback uses a direct access to the C implementation of the default hook which reduces the risk of bugs.
My implementation contains an unit test to ensure that if a custom hook raises an exception, it's a logged as expected ;-)
Victor
16.05.19 04:23, Victor Stinner пише:
The first implementation of my API used sys.unraisablehook(exc_type, exc_value, exc_tb, obj). The problem is that Serhiy Storchaka asked me to add a new error message field which breaks the API: the API is not future-proof.
I modified my API to create an object to pack arguments. The new API becomes sys.unraisablehook(unraisable) where unraisable has 4 fields: exc_type, exc_value, exc_tb, obj. This API is now future-proof: adding a new field will not break existing custom hooks!
I prefer the former design, when the hook takes 5 arguments: exc_type, exc_value, exc_tb, obj and msg. Any additional human readable information can be encoded in msg, and machine readable information can be encoded in msg or obj. Currently we have no plans for adding more details, and I do not think that we will need to do this in future. Packing arguments into a single extendable object just complicates the code and increases the chance of raising an exception or crashing.
Even obj and msg could be merged into a single object, and the hook could check whether it is a string or callable. But passing them as separate arguments is fine to me, I just think that no more complication is necessary.
Le jeu. 16 mai 2019 à 08:39, Serhiy Storchaka storchaka@gmail.com a écrit :
16.05.19 04:23, Victor Stinner пише:
The first implementation of my API used sys.unraisablehook(exc_type, exc_value, exc_tb, obj). The problem is that Serhiy Storchaka asked me to add a new error message field which breaks the API: the API is not future-proof.
I modified my API to create an object to pack arguments. The new API becomes sys.unraisablehook(unraisable) where unraisable has 4 fields: exc_type, exc_value, exc_tb, obj. This API is now future-proof: adding a new field will not break existing custom hooks!
I prefer the former design, when the hook takes 5 arguments: exc_type, exc_value, exc_tb, obj and msg. Any additional human readable information can be encoded in msg, and machine readable information can be encoded in msg or obj. Currently we have no plans for adding more details, and I do not think that we will need to do this in future. Packing arguments into a single extendable object just complicates the code and increases the chance of raising an exception or crashing.
Even obj and msg could be merged into a single object, and the hook could check whether it is a string or callable. But passing them as separate arguments is fine to me, I just think that no more complication is necessary.
I came to the UnraisableHookArgs structseq idea because of my bad experience with extension warnings "hooks". Let me tell you this story :-)
To enhance ResourceWarning messages, I modified Python 3.6 to add a new "source" parameter to the warnings module. It's the object which caused the ResourceWarning. This object is mostly used to display the traceback where the object has been allocated, traceback read using tracemalloc.get_object_traceback() and so required tracemalloc to be enabled.
https://bugs.python.org/issue26604
The problem is that the 2 warnings "hooks" use a fixed number of positional (and positional-or-keyword) parameters:
def showwarning(message, category, filename, lineno, file=None, line=None): ... def formatwarning(message, category, filename, lineno, line=None): ...
Adding a new parameter would simply break *all* custom hooks in the wild...
At the same time, internally, the warnings module uses a WarningMessage object to "pack" all parameters into a single object. I was able to easily add a new "source" attribute to WarningMessage without breaking the code using WarningMessage.
I added new hooks which accept WarningMessage:
def _showwarnmsg(msg): ... def _formatwarnmsg(msg): ...
The problem is to keep the backward compatibility: decide which hook should be called... How to detect that showwarning or _showwarnmsg has been overriden? If both are overriden, which one should be modified?
The implementation is tricky, and it caused a few minor regressions:
https://github.com/python/cpython/commit/be7c460fb50efe3b88a00281025d76acc62...
All these problems are the consequence of the initial design choice of warnings hooks: using a fixed number of parameters. This API simply cannot be extended. IMHO it's a bad design, and we can do better: WarningMessage is a better design.
--
I would prefer to not reproduce the same API design mistake with sys.unraisablehook.
I modified my PR to only use 4 fields: (exc_type, exc_value, exc_tb, obj).
I plan to add a 5th field "err_msg" ("error_message"?) later to enhance the hook (allow to pass a custom error message to give more context where the exception comes from). Passing a single UnraisableHookArgs parameter to sys.unraisablehook allows to add a 5th field without breaking the backward compatibility.
Any additional human readable information can be encoded in msg, and machine readable information can be encoded in msg or obj.
IMHO it's very risky to declare the current API as "complete". In my experience, we always want to enhance an API at some point to pass more information. IMHO the warnings module with my new "source" parameter is a good example.
For unraisable hook, it's not hard to imagine other useful parameters that can be passed to the hook to provide more context about the exception. For example, right now we only pass one object. But there are cases where a second object would totally makes sense.
--
Packing arguments into a single extendable object just complicates the code and increases the chance of raising an exception or crashing.
Technically, UnraisableHookArgs is basically a tuple of 4 items. I consider that there is a low risk that creating the object can fail.
UnraisableHookArgs creation failure *is* covered by my implementation! The default hook is called directly in C without using a temporary UnraisableHookArgs object. The exception which caused the failure is logged as well.
IMHO the very little risk of UnraisableHookArgs creation failure is worth it, compared to the pain to extend an API based on a fixed number of parameters.
Victor
After the detailed explanation, UnraisableHookArgs makes sense. Forward-compatibility is important thing
On Thu, May 16, 2019 at 1:12 PM Victor Stinner vstinner@redhat.com wrote:
Le jeu. 16 mai 2019 à 08:39, Serhiy Storchaka storchaka@gmail.com a écrit :
16.05.19 04:23, Victor Stinner пише:
The first implementation of my API used sys.unraisablehook(exc_type, exc_value, exc_tb, obj). The problem is that Serhiy Storchaka asked me to add a new error message field which breaks the API: the API is not future-proof.
I modified my API to create an object to pack arguments. The new API becomes sys.unraisablehook(unraisable) where unraisable has 4 fields: exc_type, exc_value, exc_tb, obj. This API is now future-proof: adding a new field will not break existing custom hooks!
I prefer the former design, when the hook takes 5 arguments: exc_type, exc_value, exc_tb, obj and msg. Any additional human readable information can be encoded in msg, and machine readable information can be encoded in msg or obj. Currently we have no plans for adding more details, and I do not think that we will need to do this in future. Packing arguments into a single extendable object just complicates the code and increases the chance of raising an exception or crashing.
Even obj and msg could be merged into a single object, and the hook could check whether it is a string or callable. But passing them as separate arguments is fine to me, I just think that no more complication is necessary.
I came to the UnraisableHookArgs structseq idea because of my bad experience with extension warnings "hooks". Let me tell you this story :-)
To enhance ResourceWarning messages, I modified Python 3.6 to add a new "source" parameter to the warnings module. It's the object which caused the ResourceWarning. This object is mostly used to display the traceback where the object has been allocated, traceback read using tracemalloc.get_object_traceback() and so required tracemalloc to be enabled.
https://bugs.python.org/issue26604
The problem is that the 2 warnings "hooks" use a fixed number of positional (and positional-or-keyword) parameters:
def showwarning(message, category, filename, lineno, file=None, line=None): ... def formatwarning(message, category, filename, lineno, line=None): ...
Adding a new parameter would simply break *all* custom hooks in the wild...
At the same time, internally, the warnings module uses a WarningMessage object to "pack" all parameters into a single object. I was able to easily add a new "source" attribute to WarningMessage without breaking the code using WarningMessage.
I added new hooks which accept WarningMessage:
def _showwarnmsg(msg): ... def _formatwarnmsg(msg): ...
The problem is to keep the backward compatibility: decide which hook should be called... How to detect that showwarning or _showwarnmsg has been overriden? If both are overriden, which one should be modified?
The implementation is tricky, and it caused a few minor regressions:
https://github.com/python/cpython/commit/be7c460fb50efe3b88a00281025d76acc62...
All these problems are the consequence of the initial design choice of warnings hooks: using a fixed number of parameters. This API simply cannot be extended. IMHO it's a bad design, and we can do better: WarningMessage is a better design.
--
I would prefer to not reproduce the same API design mistake with sys.unraisablehook.
I modified my PR to only use 4 fields: (exc_type, exc_value, exc_tb, obj).
I plan to add a 5th field "err_msg" ("error_message"?) later to enhance the hook (allow to pass a custom error message to give more context where the exception comes from). Passing a single UnraisableHookArgs parameter to sys.unraisablehook allows to add a 5th field without breaking the backward compatibility.
Any additional human readable information can be encoded in msg, and machine readable information can be encoded in msg or obj.
IMHO it's very risky to declare the current API as "complete". In my experience, we always want to enhance an API at some point to pass more information. IMHO the warnings module with my new "source" parameter is a good example.
For unraisable hook, it's not hard to imagine other useful parameters that can be passed to the hook to provide more context about the exception. For example, right now we only pass one object. But there are cases where a second object would totally makes sense.
--
Packing arguments into a single extendable object just complicates the code and increases the chance of raising an exception or crashing.
Technically, UnraisableHookArgs is basically a tuple of 4 items. I consider that there is a low risk that creating the object can fail.
UnraisableHookArgs creation failure *is* covered by my implementation! The default hook is called directly in C without using a temporary UnraisableHookArgs object. The exception which caused the failure is logged as well.
IMHO the very little risk of UnraisableHookArgs creation failure is worth it, compared to the pain to extend an API based on a fixed number of parameters.
Victor
Night gathers, and now my watch begins. It shall not end until my death. _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/andrew.svetlov%40gmail.co...
16.05.19 13:12, Victor Stinner пише:
I came to the UnraisableHookArgs structseq idea because of my bad experience with extension warnings "hooks". Let me tell you this story :-)
I know this story, because I followed these issues.
For unraisable hook, it's not hard to imagine other useful parameters that can be passed to the hook to provide more context about the exception. For example, right now we only pass one object. But there are cases where a second object would totally makes sense.
If you have plans for adding new details in future, I propose to add a 6th parameter "context" or "extra" (always None currently). It is as extensible as packing all arguments into a single structure, but you do not need to introduce the structure type and create its instance until you need to pass additional info.
Le jeu. 16 mai 2019 à 12:57, Serhiy Storchaka storchaka@gmail.com a écrit :
For unraisable hook, it's not hard to imagine other useful parameters that can be passed to the hook to provide more context about the exception. For example, right now we only pass one object. But there are cases where a second object would totally makes sense.
If you have plans for adding new details in future, I propose to add a 6th parameter "context" or "extra" (always None currently). It is as extensible as packing all arguments into a single structure, but you do not need to introduce the structure type and create its instance until you need to pass additional info.
In my implementation, UnraisableHookArgs is a C "structseq" type. In short, it's a tuple. make_unraisable_hook_args() basically builds a tuple of 4 items and uses PyTuple_SET_ITEM() to set the items. _PyErr_WriteUnraisableDefaultHook() uses PyStructSequence_GET_ITEM() to get items.
The code pack and unpack UnraisableHookArgs is simple and reliable.
An "unraisable exception" doesn't mean that Python is collapsing. It only means that the code is unable to report the exception to the caller: there is no reason why allocating a 4-tuple or calling a simple Python function (sys.unraisablehook) would fail.
--
I dislike the compromise of having an API in 2 parts: positional parameters for some parameters, and a structure for some other parameters. I prefer to pack all arguments into a single structure.
--
IMHO it's readable to get attributes from an object in a Python hook: it doesn't surprised me, OOP is common in Python :-) Simplified example of a pure Python reimplementation of the default hook:
def myhook(unraisable): if unraisable.obj is not None: print(f"Exception ignored in: {unraisable.obj!r}")
if unraisable.exc_tb is not None: traceback.print_tb(unraisable.exc_tb)
print(f"{unraisable.exc_type.__name__}: {unraisable.exc_value}")
Compared to positional arguments:
def myhook(exc_type, exc_value, exc_tb, obj, extras): if obj is not None: print(f"Exception ignored in: {obj!r}")
if exc_tb is not None: print_tb(exc_tb, file=file)
print(f"{exc_type.__name__}: {exc_value}")
Again, the warnings module uses a similar WarningsMsg structure and I'm not shocked by the implementation. Simplified code from Lib/warnings.py:
def _formatwarnmsg_impl(msg): category = msg.category.__name__ s = f"{msg.filename}:{msg.lineno}: {category}: {msg.message}\n"
if msg.line is None: line = linecache.getline(msg.filename, msg.lineno) else: line = msg.line if line: s += " %s\n" % line.strip()
if msg.source is not None: ... return s
Victor
On 16May2019 0647, Victor Stinner wrote:
Le jeu. 16 mai 2019 à 12:57, Serhiy Storchaka storchaka@gmail.com a écrit :
For unraisable hook, it's not hard to imagine other useful parameters that can be passed to the hook to provide more context about the exception. For example, right now we only pass one object. But there are cases where a second object would totally makes sense.
If you have plans for adding new details in future, I propose to add a 6th parameter "context" or "extra" (always None currently). It is as extensible as packing all arguments into a single structure, but you do not need to introduce the structure type and create its instance until you need to pass additional info.
In my implementation, UnraisableHookArgs is a C "structseq" type. In short, it's a tuple. make_unraisable_hook_args() basically builds a tuple of 4 items and uses PyTuple_SET_ITEM() to set the items. _PyErr_WriteUnraisableDefaultHook() uses PyStructSequence_GET_ITEM() to get items.
The code pack and unpack UnraisableHookArgs is simple and reliable.
An "unraisable exception" doesn't mean that Python is collapsing. It only means that the code is unable to report the exception to the caller: there is no reason why allocating a 4-tuple or calling a simple Python function (sys.unraisablehook) would fail.
--
I dislike the compromise of having an API in 2 parts: positional parameters for some parameters, and a structure for some other parameters. I prefer to pack all arguments into a single structure.
I really like this API, and I agree with Victor that we don't really need more than the exception info. For future expansion, we can pass in a different exception, no?
I'm not even convinced we need the obj argument, or that it's a good idea - this is yet another place where it's likely to be dead/dying already. And what can you do with it? Resurrection seems like a really bad idea, as does diving into a custom __repr__. There's no useful recovery mechanism here that I'm aware of, so I'd be in favor of just passing through the exception and nothing else.
Cheers, Steve
On 16May2019 0856, Steve Dower wrote:
On 16May2019 0647, Victor Stinner wrote:
Le jeu. 16 mai 2019 à 12:57, Serhiy Storchaka storchaka@gmail.com a écrit :
For unraisable hook, it's not hard to imagine other useful parameters that can be passed to the hook to provide more context about the exception. For example, right now we only pass one object. But there are cases where a second object would totally makes sense.
If you have plans for adding new details in future, I propose to add a 6th parameter "context" or "extra" (always None currently). It is as extensible as packing all arguments into a single structure, but you do not need to introduce the structure type and create its instance until you need to pass additional info.
In my implementation, UnraisableHookArgs is a C "structseq" type. In short, it's a tuple. make_unraisable_hook_args() basically builds a tuple of 4 items and uses PyTuple_SET_ITEM() to set the items. _PyErr_WriteUnraisableDefaultHook() uses PyStructSequence_GET_ITEM() to get items.
The code pack and unpack UnraisableHookArgs is simple and reliable.
An "unraisable exception" doesn't mean that Python is collapsing. It only means that the code is unable to report the exception to the caller: there is no reason why allocating a 4-tuple or calling a simple Python function (sys.unraisablehook) would fail.
--
I dislike the compromise of having an API in 2 parts: positional parameters for some parameters, and a structure for some other parameters. I prefer to pack all arguments into a single structure.
I really like this API, and I agree with Victor that we don't really need more than the exception info. For future expansion, we can pass in a different exception, no?
I'm not even convinced we need the obj argument, or that it's a good idea - this is yet another place where it's likely to be dead/dying already. And what can you do with it? Resurrection seems like a really bad idea, as does diving into a custom __repr__. There's no useful recovery mechanism here that I'm aware of, so I'd be in favor of just passing through the exception and nothing else.
Actually, if the default implementation prints the exception message, how is this different from sys.excepthook? Specifically, from the point of customizing the hooks.
If I were going to replace unraisablehook to do something on specific exceptions, I'm almost certainly going to replace excepthook as well, no? The only difference is whether there are any try/except blocks in between, and by definition I think the answer is no (in both cases, or we wouldn't have reached the hook).
Do you have scenarios in mind where you would need these to do different things? And would those be just as well served by a flag on the exception object rather than an entirely separate hook?
Cheers, Steve
On 16May2019 0902, Steve Dower wrote:
Actually, if the default implementation prints the exception message, how is this different from sys.excepthook? Specifically, from the point of customizing the hooks.
If I were going to replace unraisablehook to do something on specific exceptions, I'm almost certainly going to replace excepthook as well, no? The only difference is whether there are any try/except blocks in between, and by definition I think the answer is no (in both cases, or we wouldn't have reached the hook).
Do you have scenarios in mind where you would need these to do different things? And would those be just as well served by a flag on the exception object rather than an entirely separate hook?
Finally, the changes aren't in yet but PEP 578 is accepted, so perhaps this could also be satisfied by an audit hook for an exception that's about to be ignored?
Cheers, Steve
On Thu, May 16, 2019, 09:07 Steve Dower steve.dower@python.org wrote:
Actually, if the default implementation prints the exception message, how is this different from sys.excepthook? Specifically, from the point of customizing the hooks.
sys.excepthook means the program has fully unwound and is about to exit. This is pretty different from an exception inside a __del__ or background thread or whatever, where the program definitely hasn't unwound and is likely to continue. And I'm pretty sure they have behavioral differences already, like if you pass in a SystemExit exception then sys.excepthook doesn't print anything, but PyErr_WriteUnraiseable prints a traceback.
So making them two separate hooks seems right to me. Some people will override both; that's fine.
-n
Le jeu. 16 mai 2019 à 17:56, Steve Dower steve.dower@python.org a écrit :
I really like this API, and I agree with Victor that we don't really need more than the exception info. For future expansion, we can pass in a different exception, no?
Sorry, I don't understand. I explained that we need more than (exc_type, exc_value, exc_tb).
"obj" is part of the C function PyErr_WriteUnraisable(). We have to pass it to the hook. Otherwise, how do you want to guess where the exception comes from? Without any context, it can be super painful to debug such exception :-(
That's why I said that I like Serhiy's idea of extending the API to allow to also pass an error message.
Unraisable exceptions are commonly raised during garbage collection or in finalizers. So it's commonly happens when you don't "expect" them :-)
I'm not even convinced we need the obj argument, or that it's a good idea - this is yet another place where it's likely to be dead/dying already. And what can you do with it? Resurrection seems like a really bad idea, as does diving into a custom __repr__. There's no useful recovery mechanism here that I'm aware of, so I'd be in favor of just passing through the exception and nothing else.
Well, first of all, I would advice to *not* keep "obj" alive after the execution of the hook. Keep repr(obj) if you want, but not a reference to the object. Same for the exception value, keeping it alive is a high risk of creating annoying reference cycles ;-)
--
In a finalizer, "obj" is not always the object being finalized.
Example: when an _asyncio.Future is finalized, loop.call_exception_handler() is called if the future wasn't consumed. If this call fails, PyErr_WriteUnraisable(func) is called. func isn't the future, but the call_exception_handler() method.
Example: on a garbage collection, callbacks of weak references are called. If a callback fails, PyErr_WriteUnraisable(callback) is called. It's not the collected object nor the weak reference.
It's also common that PyErr_WriteUnraisable(NULL) is called. In this case, you have no context where the exception comes from :-( For that, I experimented a custom hook which logs also the current Python stack. That's useful in many cases!
socket.socket finalizer is a case where the finalized object (the socket) is passed to the hook. IMHO it's fine to resurrect a socket in that case. Finalization isn't deallocation. The objet remains consistent. The finalization protocol ensures that the finalizer is only called once.
In practice, I wouldn't advice to resurrect objects :-) I would expect that a hook only calls repr(hook) and then forgets the object.
The current PyErr_WriteUnraisable() implementation does exactly that: it formats repr(obj) into sys.stderr.
*If* someone finds a case where PyErr_WriteUnraisable() can resurect an object that is freed just after the call, I would suggest to fix the code to not pass the object to PyErr_WriteUnraisable(). Using PEP 442 finalizers, I don't think that it should happen.
Victor
On 16May2019 1404, Victor Stinner wrote:
Le jeu. 16 mai 2019 à 17:56, Steve Dower steve.dower@python.org a écrit :
I really like this API, and I agree with Victor that we don't really need more than the exception info. For future expansion, we can pass in a different exception, no?
Sorry, I don't understand. I explained that we need more than (exc_type, exc_value, exc_tb).
"obj" is part of the C function PyErr_WriteUnraisable(). We have to pass it to the hook. Otherwise, how do you want to guess where the exception comes from? Without any context, it can be super painful to debug such exception :-(
You go on to say "pass an error message" and "keep repr(obj) if you want", but how is this different from creating an exception that contains the custom message, the repr of the object, and chains the exception that triggered it?
If you just have a message and no exception at this point, firstly it's going against what the hook is documented as doing, but secondly why not convert the message into an exception?
Maybe I just missed where you explained how it isn't possible to represent an error message as an exception, but I'm pretty sure I can't be the only one who will have missed it, so maybe you could explain just that one point?
Thanks, Steve
Le jeu. 16 mai 2019 à 23:17, Steve Dower steve.dower@python.org a écrit :
You go on to say "pass an error message" and "keep repr(obj) if you want", but how is this different from creating an exception that contains the custom message, the repr of the object, and chains the exception that triggered it?
Well, "unraisable exceptions" are raised when something goes wrong. I'm not comfortable with creating a new exception instance and chaining it to the previous exception, because it could go wrong and raise a 3rd exception. Issue a new error while trying to log an error can be annoying :-(
Moreover, when I looked at details of the "unraisable exception" (see my reply to Petr Viktorin), I saw that the exception is not well defined as you might expect. (exc_type, exc_value, exc_tb) can *not* be replaced with (type(exc_value), exc_value, exc_value.__traceback__). Sometimes, exc_value is None. Sometimes, exc_tb is a traceback object, but exc_value.__traceback__ is None. So I'm not comfortable neither to chain such badly shaped exception into a new exception.
I prefer to pass "raw" values and let the hook decides how to handle them :-)
Said differently, I prefer to design the hook to restrict the risks of creating a new error. At least in the code which calls the hook. Inside the hook.
Victor
On 16May2019 1441, Victor Stinner wrote:
Le jeu. 16 mai 2019 à 23:17, Steve Dower steve.dower@python.org a écrit :
You go on to say "pass an error message" and "keep repr(obj) if you want", but how is this different from creating an exception that contains the custom message, the repr of the object, and chains the exception that triggered it?
Well, "unraisable exceptions" are raised when something goes wrong. I'm not comfortable with creating a new exception instance and chaining it to the previous exception, because it could go wrong and raise a 3rd exception. Issue a new error while trying to log an error can be annoying :-(
I mean, aren't they all? :) That's why they're exceptional.
If we're not in a position to raise a new exception, then it's not safe to call into user's Python code. If it's safe to call into their Python code, then we have to be in a position to raise a new exception.
You said earlier that we can safely do this, and now you're saying we can't safely do it. Which is it?
Moreover, when I looked at details of the "unraisable exception" (see my reply to Petr Viktorin), I saw that the exception is not well defined as you might expect. (exc_type, exc_value, exc_tb) can *not* be replaced with (type(exc_value), exc_value, exc_value.__traceback__). Sometimes, exc_value is None. Sometimes, exc_tb is a traceback object, but exc_value.__traceback__ is None. So I'm not comfortable neither to chain such badly shaped exception into a new exception.
Yeah, this is the normal optimization for raising and handling exceptions that never reach Python code. Exceptions are supposed to be normalized before re-entering the eval loop.
I prefer to pass "raw" values and let the hook decides how to handle them :-)
This is fine, it's how we handle exceptions in most other places (such as __exit__, logging, sys, traceback and all through the C API), and skipping normalization for a last-chance "the-world-may-be-burning" hook isn't the worst thing ever.
Cheers, Steve
I like the feature, we should have it. It'll be useful for debugging and probably more.
Which brings me to the annoying paint color question: These exceptions were most definitely raised. Thus the term "unraisable" is wrong. I believe you really mean "uncatchable".
-gps
On Thu, May 16, 2019 at 4:00 PM Steve Dower steve.dower@python.org wrote:
On 16May2019 1441, Victor Stinner wrote:
Le jeu. 16 mai 2019 à 23:17, Steve Dower steve.dower@python.org a
écrit :
You go on to say "pass an error message" and "keep repr(obj) if you want", but how is this different from creating an exception that contains the custom message, the repr of the object, and chains the exception that triggered it?
Well, "unraisable exceptions" are raised when something goes wrong. I'm not comfortable with creating a new exception instance and chaining it to the previous exception, because it could go wrong and raise a 3rd exception. Issue a new error while trying to log an error can be annoying :-(
I mean, aren't they all? :) That's why they're exceptional.
If we're not in a position to raise a new exception, then it's not safe to call into user's Python code. If it's safe to call into their Python code, then we have to be in a position to raise a new exception.
You said earlier that we can safely do this, and now you're saying we can't safely do it. Which is it?
Moreover, when I looked at details of the "unraisable exception" (see my reply to Petr Viktorin), I saw that the exception is not well defined as you might expect. (exc_type, exc_value, exc_tb) can *not* be replaced with (type(exc_value), exc_value, exc_value.__traceback__). Sometimes, exc_value is None. Sometimes, exc_tb is a traceback object, but exc_value.__traceback__ is None. So I'm not comfortable neither to chain such badly shaped exception into a new exception.
Yeah, this is the normal optimization for raising and handling exceptions that never reach Python code. Exceptions are supposed to be normalized before re-entering the eval loop.
I prefer to pass "raw" values and let the hook decides how to handle
them :-)
This is fine, it's how we handle exceptions in most other places (such as __exit__, logging, sys, traceback and all through the C API), and skipping normalization for a last-chance "the-world-may-be-burning" hook isn't the worst thing ever.
Cheers, Steve _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/greg%40krypto.org
Le ven. 17 mai 2019 à 03:10, Gregory P. Smith greg@krypto.org a écrit :
I like the feature, we should have it. It'll be useful for debugging and probably more.
Which brings me to the annoying paint color question: These exceptions were most definitely raised. Thus the term "unraisable" is wrong. I believe you really mean "uncatchable".
"unraisablehook" name comes from the existing PyErr_WriteUnraisable().
I understood that "unraisable" means that the function calling PyErr_WriteUnraisable() cannot "raise" the exception to its caller. The exception has to be "swallowed" by the function.
--
If you like the feature, please review my PR :-)
https://github.com/python/cpython/pull/13187
Victor
On Thu, May 16, 2019 at 2:17 PM Steve Dower steve.dower@python.org wrote:
You go on to say "pass an error message" and "keep repr(obj) if you want", but how is this different from creating an exception that contains the custom message, the repr of the object, and chains the exception that triggered it?
A clever hook might want the actual object, so it can pretty-print it, or open an interactive debugger and let it you examine it, or something. Morally this is similar to calling repr(obj), but it doesn't literally call repr(obj).
-n
Le jeu. 16 mai 2019 à 23:46, Nathaniel Smith njs@pobox.com a écrit :
A clever hook might want the actual object, so it can pretty-print it, or open an interactive debugger and let it you examine it, or something. Morally this is similar to calling repr(obj), but it doesn't literally call repr(obj).
Good point. By the way, I started to use tracemalloc.get_object_traceback(obj) to display where the object comes from more and more often :-D It helps me at least in debugging. So yeah, I would prefer to pass directly the object to the hook.
For example, I'm using it with the "source" parameter of a ResourceWarning: https://pythondev.readthedocs.io/debug_tools.html#resourcewarning
But also when a buffer underflow or overflow is detected by debug hooks on Python memory allocators: https://pythondev.readthedocs.io/debug_tools.html#pythonmalloc-debug
It might also help me sometimes to be able to call tracemalloc.get_object_traceback(obj) from a custom unraisablehook().
Victor
On May 16, 2019, at 03:12, Victor Stinner vstinner@redhat.com wrote:
I came to the UnraisableHookArgs structseq idea because of my bad experience with extension warnings "hooks". Let me tell you this story :-)
Thanks for that!
def showwarning(message, category, filename, lineno, file=None, line=None): ... def formatwarning(message, category, filename, lineno, line=None): …
While I’m fine with the API you propose, you could consider keyword-only arguments to help future proof the call signature. I don’t like it as much because it’s not as backward compatibility proof, but it’s an option to perhaps consider.
-Barry
While I’m fine with the API you propose, you could consider keyword-only
arguments to help future proof the call signature. I don’t like it as much because it’s not as backward compatibility proof, but it’s an option to perhaps consider.
Let's say that we have: hook(*, exc_type, exc_value, exc_tb, obj). Now we pass a new err_msg keyword argument: the call fails with a TypeError :-(
Or do you mean using **kwargs? I don't think **kwargs is a good API :-( Well, the same can be done using *args and positional arguments to ignore new unknown arguments, but I also dislike such API.
Victor
On 5/16/19 3:23 AM, Victor Stinner wrote: [...]
I modified my API to create an object to pack arguments. The new API becomes sys.unraisablehook(unraisable) where unraisable has 4 fields: exc_type, exc_value, exc_tb, obj.
[...] I always thought the classic (exc_type, exc_value, exc_tb) triple is a holdover from older Python versions, and all the information is now in the exception instance. Is the triple ever different from (type(exc), exc, exc.__traceback__)? (possibly with a getattr for __traceback__) Should new APIs use it?
Le jeu. 16 mai 2019 à 20:58, Petr Viktorin encukou@gmail.com a écrit :
I always thought the classic (exc_type, exc_value, exc_tb) triple is a holdover from older Python versions, and all the information is now in the exception instance. Is the triple ever different from (type(exc), exc, exc.__traceback__)? (possibly with a getattr for __traceback__)
I added assertions in PyErr_WriteTraceback():
assert(Py_TYPE(v) == t); assert(PyException_GetTraceback(v) == tb);
"Py_TYPE(v) == t" fails in test_exceptions.test_memory_error_in_PyErr_PrintEx() for example. PyErr_NoMemory() calls PyErr_SetNone(PyExc_MemoryError), it sets tstate->curexc_type to PyExc_MemoryError, but tstate->curexc_value is set to NULLL.
"PyException_GetTraceback(v) == tb" fails in test_exceptions.test_unraisable() for example: "PyTraceBack_Here(f);" in the "error:" label of ceval.c creates a traceback object and sets it to tstate->curexec_traceback, but it doesn't set the __traceback__ attribute of the current exception.
Should new APIs use it?
I tried to add a "PyErr_NormalizeException(&t, &v, &tb);" call in PyErr_WriteUnraisable(): it creates an exception object (exc_value) for the PyErr_NoMemory() case, but it still doesn't set the __traceback__ attribute of the exception for the PyTraceBack_Here() case.
It seems like PyErr_WriteUnraisable() cannot avoid having 3 variables (exc_type, exc_value, exc_tb), since they are not consistent as you may expect.
Victor
On Thu, May 16, 2019 at 1:23 PM Victor Stinner vstinner@redhat.com wrote:
Le jeu. 16 mai 2019 à 20:58, Petr Viktorin encukou@gmail.com a écrit :
I always thought the classic (exc_type, exc_value, exc_tb) triple is a holdover from older Python versions, and all the information is now in the exception instance. Is the triple ever different from (type(exc), exc, exc.__traceback__)? (possibly with a getattr for __traceback__)
I added assertions in PyErr_WriteTraceback():
assert(Py_TYPE(v) == t); assert(PyException_GetTraceback(v) == tb);"Py_TYPE(v) == t" fails in test_exceptions.test_memory_error_in_PyErr_PrintEx() for example. PyErr_NoMemory() calls PyErr_SetNone(PyExc_MemoryError), it sets tstate->curexc_type to PyExc_MemoryError, but tstate->curexc_value is set to NULLL.
This makes some sense – if you can't allocate memory, then better not allocate an exception instance to report that! So this is legitimately a special case.
But... it looks like everywhere else, the way we handle this when transitioning into Python code is to create an instance. For example, that test does 'except MemoryError as e', so an instance does need to be created then. The comments suggest that there's some trick where we have pre-allocated MemoryError() instances? But either way, if we can afford to call a Python hook (which requires at least allocating a frame!), then we can probably also afford to materialize the MemoryError instance. So I feel like maybe we shouldn't be passing None to the unraisable hook, even if PyErr_NoMemory() did initially set that?
Also, in practice, the only time I've ever seen MemoryError is from attempting to do a single massively-huge allocation. It's never meant that regular allocation of small objects will fail.
"PyException_GetTraceback(v) == tb" fails in test_exceptions.test_unraisable() for example: "PyTraceBack_Here(f);" in the "error:" label of ceval.c creates a traceback object and sets it to tstate->curexec_traceback, but it doesn't set the __traceback__ attribute of the current exception.
Isn't this just a bug that should be fixed?
Should new APIs use it?
I tried to add a "PyErr_NormalizeException(&t, &v, &tb);" call in PyErr_WriteUnraisable(): it creates an exception object (exc_value) for the PyErr_NoMemory() case, but it still doesn't set the __traceback__ attribute of the exception for the PyTraceBack_Here() case.
It seems like PyErr_WriteUnraisable() cannot avoid having 3 variables (exc_type, exc_value, exc_tb), since they are not consistent as you may expect.
I'm actually fine with it having three arguments -- even if it's technically unnecessary, it's currently 100% consistent across these low-level APIs, and it doesn't hurt anything, so we might as well stick with it for consistency.
-n
-
Andrew Svetlov -
Barry Warsaw -
Gregory P. Smith -
Nathaniel Smith -
Petr Viktorin -
Serhiy Storchaka -
Steve Dower -
Victor Stinner