Skip to content
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

GH-96704: Add task.get_context(), use it in call_exception_handler() #96756

Merged
merged 7 commits into from
Oct 5, 2022

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants