Skip to content
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! Update Doc/c-api/exceptions.rst
  • Loading branch information
pablogsal committed Aug 17, 2021
commit 30135a1b0d4183b9490bed76e6d69324e5305896
20 changes: 11 additions & 9 deletions Doc/c-api/exceptions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -127,22 +127,24 @@ For convenience, some of these functions will always return a

.. c:function:: void PyErr_ChainExceptions(PyObject *exc, PyObject *val, PyObject *tb)

Set the provided exception as the cause for the current exception. If there is no exception
currently set (:c:func:`PyErr_Occurred` returns ``0``) this function behaves
as like :c:func:`PyErr_Restore`, but if an exception is already set, set the
context associated with it.
Set the provided exception as the cause for the current exception. If there
Comment thread
pablogsal marked this conversation as resolved.
Outdated
is no exception currently set (:c:func:`PyErr_Occurred` returns ``0``) this
function behaves as like :c:func:`PyErr_Restore`, but if an exception is
Comment thread
pablogsal marked this conversation as resolved.
Outdated
already set, set the provided exception as the context (accessible from
Python through the :attr:`__context__` attribute of the exception) of the
one that is currently set.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it the other way around - the existing exception becomes the context of the provided exception?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code calls PyException_SetContext(val2, val); where val is the provided one and val2 is the one that is currently set. PyException_SetContext sets the second argument as the context for the first.

So unless I am missing something (is late here 😅 ) this means that "The provided exception (val) becomes the context of the existing exception (val2)"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I was thinking we are raising the provided exception in the context of the existing one, but that's what SetObject is for. Can we think of a more descriptive name for this function? I confused the direction, and you confused cause/context because "chain" is ambiguous here.

How about PyErr_RestoreContext? IIUC this describes what you use this function for - putting back a fetched exception, as the context of any exception that happened between the fetch and now. (Should something like the previous sentence be in the doc?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or possibly PyErr_RestoreAsContext?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general authors look for "chain" in the docs because that's the action they want to do, so I would really recommend to have "chain" in the name.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may be the case, but I think we should promote clear language, and "chain" in this context is vague. The doc can contain "chain" in the prose for the benefit of people who search for it (for now). But should it be part of the function name for all eternity?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is unclear to me. I agree with the ambiguity but what users if the function want is a higher level approach to the python behaviour: an exception is raised when another exception was there and I want to do in C the same thing as Python does.

I'm not a native speaker do I trust your judgement regarding how ambiguous this sounds and that we need a better name so let's search for one.

Maybe we should make a section in the docs about "exception chaining" and offer two APIs: one for cause and another for context

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iritkatriel Maybe you can suggest a more specific name that still retains "chain" as part of it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context is an implicitly chained exception (the cause is explicitly chained). There is no need to have both chained and context in the name. If we have only one of them I think it should be context and not chain.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the equivalence to PyErr_Restore when no exception is currently set, my public naming suggestion would be PyErr_RestoreOrChain, with the shorthand documentation being "Restores the given exception state, or chains it as the exception context if an exception is already set".

The key point to emphasise is that the function takes the same arguments as PyErr_Restore (retrieved from a prior PyErr_Fetch call), and generally does the same thing, but handles currently set exceptions differently: instead of clearing them, it leaves them in place, and "restores" the given exception as the exception context, rather than as the active exception.


The exception type should be a class. Do not pass an invalid exception type or value. This
function steals a reference to each of its arguments in the same fashion as :c:func:`PyErr_Restore`
does.
The exception type should be a class. Do not pass an invalid exception type
or value. This function steals a reference to each of its arguments in the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it not steal its arguments?
Functions that don't follow the usual calling convention are harder to reason about; it would be good to treat them as historical exceptions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It steals the arguments because of what it calls. It had the same calling convention as Pyerr_Restore.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, stealing arguments is not an uncommon calling convention.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stealing references is only common because much of the public API was born as internal APIs, and we often try to avoid redundant refcount adjustments in internal APIs.

It took us a long time to realise that stealing references in public APIs makes things confusing for API clients, and can easily lead to double decref bugs (which are more potentially destructive than the object leaks that can arise from missing decref bugs, since deleting objects early can lead to access violations). So I think the new public API should be a wrapper around the internal API that avoids stealing references.

I changed my mind on this point, as PyErr_Fetch and PyErr_Restore are a matched pair. If we go with my PyErr_RestoreOrChain naming suggestion, then the refcounting semantics of the new API should match those of PyErr_Restore (which is to take back the references that PyErr_Fetch created)

same fashion as :c:func:`PyErr_Restore` does.

.. warning::

The caller is responsible for ensuring that this call won't create
any cycles in the exception context chain.

@iritkatriel iritkatriel Aug 17, 2021 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the doc should say what happens if you violate the rule. I think in reality it will just create the cycle and if it doesn't bother you then good for you.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the doc should say what happens if you violate the rule.

Is undefined behavior, technically as the contract of this function withing CPython is that cycles should not be created. Some parts of the exception machinery can break (or at least they used to) in presence of cycles.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when the exception machinery breaks in the presence of a cycle we consider that to be a bug in python.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the caller supposed to ensure this? Is there any API for checking it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no API to check this, but to create a cycle with exceptions it needs to be s deliberate action. There is not an easy way to accidentally crate an exception cycle

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when the exception machinery breaks in the presence of a cycle we consider that to be a bug in python.

I think the movement is in the direction of trying to prevent / avoid the creation of cycles in the first place (as was mentioned in the discussion for this recent ticket, for example). The warning being added here is in line with that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my comment below, the easiest way to violate this requirement is to call the API without setting a new exception first (or clearing the existing one). Buggy code (using my API naming suggestion from above):

PyErr_Fetch(&type, &value, &traceback);
// Exception state is not cleared, no new exception is set
PyErr_RestoreOrChain(type, value, traceback); // Sets exception as its own context

Because the exception might not have been instantiated yet when PyErr_Fetch() is called, there's no easy way for the API to reliably detect this error, so the onus is on the caller to manage the exception state correctly.


See :ref:`tut-exception-chaining` for more information regarding the semantics of exception
chaining.
See :ref:`tut-exception-chaining` for more information regarding the
semantics of exception chaining.

Example usage:

Expand Down