Skip to content

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 11, 2022

The point here is for call_exception_handler() to make an effort to run the exception handler in the correct task context (contextvars.Context), if it is called on behalf of a task.

The easiest way seems to be to retrieve the task context from the task passed into the context dict that's being passed to call_exception_handler(). This doesn't always exist, and because Task inherits from Future, it may be stored under either a "task" key or a "future" key. So try both.

We then need to retrieve the contextvars.Context from the task, for which I have added a get_context() method to the Task class (analogous to get_name()). It is possible that the task doesn't have this method (e.g. if the loop is using a 3rd party task implementation that doesn't yet support this new method).

Only if get_context() exists and returns something that has a run() method do we run the exception handler using that contextvars.Context.run() method. Otherwise we use the default contextvars.Context.

We only do this for an exception handler set by the user; the default exception handler should not be interested in the context. (This is because I'm lazy, there are two calls to it and I don't want to bother giving those the same treatment -- we can reconsider if you think the logger might also be interested in the contextvars.Context.)

PS. It's unfortunate that call_exception_handler() has a parameter named context that is a dict, not a contextvars.Context. But nothing we can do it now (this API probably predates contextvars).

@gvanrossum
Copy link
Member Author

Possibly we also need to look for a Context in the "handle" key of the context arg, since Handles also have a context.

An alternative would of course be to use self._context.run(loop.call_exception_handler, context) everywhere where it makes sense. But how many locations is that? And it would mean more C code -- there are two calls to call_exception_handler in _asynciomodule.c, and 3rd party task implementations would have to duplicate that code (instead of just the code for get_context(), which is pretty straightforward).

@gvanrossum
Copy link
Member Author

Possibly we also need to look for a Context in the "handle" key of the context arg, since Handles also have a context.

An alternative would be to use self._context.run(loop.call_exception_handler, context) everywhere where it makes sense. But how many locations is that? And it would mean more C code -- there are two calls to call_exception_handler in _asynciomodule.c, and 3rd party task implementations would have to duplicate that code (instead of just the code for get_context(), which is pretty straightforward).

@gvanrossum gvanrossum marked this pull request as ready for review September 14, 2022 22:03
@gvanrossum
Copy link
Member Author

We still need a mention in the docs for set_exception_handler, and similar code for Handle.

@gvanrossum
Copy link
Member Author

I hope you don't mind I am going to attempt a rebase here. I could merge main, but git log looks confusing to me when I do that. :-(

@gvanrossum gvanrossum force-pushed the exc-handler-context branch from a2a0c57 to bc3000b Compare October 4, 2022 20:47
@gvanrossum
Copy link
Member Author

@kumaraditya303 Ready for final review.

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gvanrossum gvanrossum merged commit 8079bef into python:main Oct 5, 2022
@gvanrossum gvanrossum deleted the exc-handler-context branch October 5, 2022 06:49
carljm added a commit to carljm/cpython that referenced this pull request Oct 6, 2022
* main: (66 commits) pythongh-65961: Raise `DeprecationWarning` when `__package__` differs from `__spec__.parent` (python#97879) docs(typing): add "see PEP 675" to LiteralString (python#97926) pythongh-97850: Remove all known instances of module_repr() (python#97876) I changed my surname early this year (python#96671) pythongh-93738: Documentation C syntax (:c:type:<C type> -> :c:expr:<C type>) (python#97768) pythongh-91539: improve performance of get_proxies_environment (python#91566) build(deps): bump actions/stale from 5 to 6 (python#97701) pythonGH-95172 Make the same version `versionadded` oneline (python#95172) pythongh-88050: Fix asyncio subprocess to kill process cleanly when process is blocked (python#32073) pythongh-93738: Documentation C syntax (Function glob patterns -> literal markup) (python#97774) pythongh-93357: Port test cases to IsolatedAsyncioTestCase, part 2 (python#97896) pythongh-95196: Disable incorrect pickling of the C implemented classmethod descriptors (pythonGH-96383) pythongh-97758: Fix a crash in getpath_joinpath() called without arguments (pythonGH-97759) pythongh-74696: Pass root_dir to custom archivers which support it (pythonGH-94251) pythongh-97661: Improve accuracy of sqlite3.Cursor.fetchone docs (python#97662) pythongh-87092: bring compiler code closer to a preprocessing-opt-assembler organisation (pythonGH-97644) pythonGH-96704: Add {Task,Handle}.get_context(), use it in call_exception_handler() (python#96756) pythongh-93738: Documentation C syntax (:c:type:`PyTypeObject*` -> :c:expr:`PyTypeObject*`) (python#97778) pythongh-97825: fix AttributeError when calling subprocess.check_output(input=None) with encoding or errors args (python#97826) Add re.VERBOSE flag documentation example (python#97678) ...
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
…tion_handler() (python#96756) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants