gh-124594: Create and reuse one context for the entire asyncio REPL session #124595

Sep 26, 2024 · 2024-09-26T12:28:39Z

Fixes gh-124594.
Closes gh-87585.

I was initially slightly unsure how to test it, and spawn_repl("-m", "asyncio") is proven to be quirky (launch ./python -I -i -m asyncio by yourself and then type in exit and see what happens)—please let me know if there's a better way.

I used copy_context() to assign the copied context to the AsyncIOInteractiveConsole.context attribute, and didn't make a context parameter as I didn't see that necessary (it doesn't really seem to be a part of the public API, even though it can be imported and used by anyone, and it's also not in the typeshed).

I believe copy_context() makes more sense than manually constructing the context with Context() because the pythonstartup.py script can set some context variables, but I'm open to discussion about that.

Eclips4 reacted with heart emoji

cpython-cla-bot · 2024-09-26T12:28:43Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-09-26T12:28:46Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Misc/NEWS.d/next/Library/2024-09-26-13-43-39.gh-issue-124594.peYhsP.rst

ZeroIntensity · 2024-09-26T14:05:27Z

This needs a NEWS entry, per the bot, as this is a user-facing bug. (Also, hi!)

Eclips4 · 2024-09-26T14:12:04Z

Is this specific to a new REPL, or is it also a problem with the old REPL? If so, there's no need to backport it to 3.12.

ZeroIntensity · 2024-09-26T14:19:32Z

Is this specific to a new REPL, or is it also a problem with the old REPL? If so, there's no need to backport it to 3.12.

Oh, I didn't check. Does the asyncio REPL use the new REPL?

ZeroIntensity · 2024-09-26T14:20:11Z

This needs a NEWS entry, per the bot, as this is a user-facing bug. (Also, hi!)

Nevermind, I was being dumb, there's already a news entry. Ignore that!

Eclips4 · 2024-09-26T14:34:55Z

Is this specific to a new REPL, or is it also a problem with the old REPL? If so, there's no need to backport it to 3.12.

Oh, I didn't check. Does the asyncio REPL use the new REPL?

Yes, it is.

Sep 27, 2024 · 2024-09-27T12:38:41Z

Another fun fact I've also observed is that user_inputX = ... variables used in tests are not in fact atomic user inputs, in contrast to the analogous string variables in the standard REPL's tests, and every line break in them causes an immediate prompt EOF (try to indent the body of the set_var() function and see what happens – ... <rest of input> behavior doesn't really seem to get triggered, maybe it's something PTY-related?). I'm not sure how to handle that; it seems to be a part of a separate bug triggered by -i and -m asyncio used together in the circumstances of spawn_repl().

every line break in them causes an immediate prompt EOF (try to indent the body of the set_var() function and see what happens – ... <rest of input> behavior doesn't really seem to get triggered, maybe it's something PTY-related?)

$ ./python -m Lib.test.test_repl F........... ====================================================================== FAIL: test_toplevel_contextvars_async (__main__.TestAsyncioREPLContextVars.test_toplevel_contextvars_async) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/bswck/Projects/cpython/Lib/test/test_repl.py", line 328, in test_toplevel_contextvars_async self.assertIn(expected, output, expected) ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: 'toplevel contextvar test: ok' not found in 'asyncio REPL 3.14.0a0 (heads/main-dirty:8447c933da3, Sep 25 2024, 15:45:56) [GCC 11.4.0] on linux\nUse "await" directly instead of "asyncio.run()".\nType "help", "copyright", "credits" or "license" for more information.\n>>> import asyncio\n>>> >>> >>> File "<stdin>", line 1\n async def set_var():\n ^\nIndentationError: expected an indented block after function definition on line 1\n>>> File "<stdin>", line 1\n var.set(\'ok\')\nIndentationError: unexpected indent\n>>> Traceback (most recent call last):\n File "/home/bswck/Projects/cpython/Lib/concurrent/futures/_base.py", line 448, in result\n return self.__get_result()\n ~~~~~~~~~~~~~~~~~^^\n File "/home/bswck/Projects/cpython/Lib/concurrent/futures/_base.py", line 393, in __get_result\n raise self._exception\n File "<stdin>", line 1, in <module>\nNameError: name \'set_var\' is not defined\n>>> toplevel contextvar test: failed\n>>> \nexiting asyncio REPL...\nTraceback (most recent call last):\n File "<frozen runpy>", line 198, in _run_module_as_main\n File "<frozen runpy>", line 88, in _run_code\n File "/home/bswck/Projects/cpython/Lib/asyncio/__main__.py", line 204, in <module>\n sys.exit(return_code)\n ~~~~~~~~^^^^^^^^^^^^^\nSystemExit: 0\n>>> \n' : toplevel contextvar test: ok ---------------------------------------------------------------------- Ran 12 tests in 0.353s FAILED (failures=1)

(This failure is a demo of a secondary concern and is effectively worked around in this PR.)

ZeroIntensity

I manually confirmed locally that this fixes the problem. FWIW, @Eclips4, I checked that this does apply to 3.12, so we're fine with the backport.

kumaraditya303 · 2024-09-29T16:01:35Z

I believe copy_context() makes more sense than manually constructing the context with Context() because the pythonstartup.py script can set some context variables, but I'm open to discussion about that.

I haven't followed the new repl change so can't review but copy_context doesn't allows back propagation across threads, not sure if that matters here.

ZeroIntensity · 2024-09-29T16:04:13Z

As far as I can see, this isn't specific to the new REPL.

asvetlov · 2024-10-01T13:40:34Z

LGTM, thanks!

miss-islington-app · 2024-10-01T14:17:26Z

Thanks @bswck for the PR, and @asvetlov for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

…ncio REPL session (pythonGH-124595) (cherry picked from commit 67e01a4) Co-authored-by: Bartosz Sławecki <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

bedevere-app · 2024-10-01T14:17:47Z

GH-124848 is a backport of this pull request to the 3.13 branch.

bedevere-app · 2024-10-01T14:17:58Z

GH-124849 is a backport of this pull request to the 3.12 branch.

Oct 1, 2024 · 2024-10-01T15:11:57Z

@asvetlov Thanks! This also closes out gh-87585 and gh-24773.

…yncio REPL session (GH-124595) (#124848) gh-124594: Create and reuse the same context for the entire asyncio REPL session (GH-124595) (cherry picked from commit 67e01a4) Co-authored-by: Bartosz Sławecki <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

…yncio REPL session (GH-124595) (#124849) * gh-124594: Create and reuse the same context for the entire asyncio REPL session (GH-124595) (cherry picked from commit 67e01a4) Co-authored-by: Bartosz Sławecki <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]> --------- Co-authored-by: Bartosz Sławecki <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

September 26, 2024 14:20

Create and reuse one context for the entire asyncio REPL session

da33967

Add NEWS entry

ea27b64

1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners September 26, 2024 12:28

bedevere-app bot added the awaiting review label Sep 26, 2024

bedevere-app bot mentioned this pull request Sep 26, 2024

Every input in asyncio REPL (wrongly) runs in a separate PEP 567 context #124594

Closed

force-pushed the fix-toplevel-contextvars-in-asyncio-repl branch from b68b941 to ea27b64 Compare September 26, 2024 13:10

ZeroIntensity reviewed Sep 26, 2024

View reviewed changes

Misc/NEWS.d/next/Library/2024-09-26-13-43-39.gh-issue-124594.peYhsP.rst Outdated Show resolved Hide resolved

ZeroIntensity added topic-asyncio needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Sep 26, 2024

Reformat the NEWS entry and add attribution

a626ea8

force-pushed the fix-toplevel-contextvars-in-asyncio-repl branch from c475f7b to a626ea8 Compare September 26, 2024 15:54

kumaraditya303 requested a review from ambv September 27, 2024 12:09

ZeroIntensity approved these changes Sep 29, 2024

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Sep 29, 2024

Sep 30, 2024

bpo-43419: fix contextvars behaviors in asyncio REPL #24773

Closed

asvetlov approved these changes Oct 1, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Oct 1, 2024

Merge branch 'main' into fix-toplevel-contextvars-in-asyncio-repl

aa6de5d

asvetlov enabled auto-merge (squash) October 1, 2024 13:42

asvetlov merged commit 67e01a4 into python:main Oct 1, 2024
34 checks passed

bedevere-app bot removed the awaiting merge label Oct 1, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 1, 2024

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 1, 2024

ZeroIntensity mentioned this pull request Oct 1, 2024

contextvars does not work properly in asyncio REPL. #87585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-124594: Create and reuse one context for the entire asyncio REPL session #124595

gh-124594: Create and reuse one context for the entire asyncio REPL session #124595

Sep 26, 2024 •

edited

Loading

cpython-cla-bot bot commented Sep 26, 2024 •

edited

Loading

bedevere-app bot commented Sep 26, 2024

ZeroIntensity commented Sep 26, 2024

Eclips4 commented Sep 26, 2024

ZeroIntensity commented Sep 26, 2024

ZeroIntensity commented Sep 26, 2024

Eclips4 commented Sep 26, 2024 •

edited

Loading

Sep 27, 2024 •

edited

Loading

ZeroIntensity left a comment

kumaraditya303 commented Sep 29, 2024

ZeroIntensity commented Sep 29, 2024

asvetlov commented Oct 1, 2024

miss-islington-app bot commented Oct 1, 2024

bedevere-app bot commented Oct 1, 2024

bedevere-app bot commented Oct 1, 2024

Oct 1, 2024

gh-124594: Create and reuse one context for the entire asyncio REPL session #124595

gh-124594: Create and reuse one context for the entire asyncio REPL session #124595

Conversation

Sep 26, 2024 • edited Loading

cpython-cla-bot bot commented Sep 26, 2024 • edited Loading

bedevere-app bot commented Sep 26, 2024

ZeroIntensity commented Sep 26, 2024

Eclips4 commented Sep 26, 2024

ZeroIntensity commented Sep 26, 2024

ZeroIntensity commented Sep 26, 2024

Eclips4 commented Sep 26, 2024 • edited Loading

Sep 27, 2024 • edited Loading

ZeroIntensity left a comment

Choose a reason for hiding this comment

kumaraditya303 commented Sep 29, 2024

ZeroIntensity commented Sep 29, 2024

asvetlov commented Oct 1, 2024

miss-islington-app bot commented Oct 1, 2024

bedevere-app bot commented Oct 1, 2024

bedevere-app bot commented Oct 1, 2024

Oct 1, 2024

Sep 26, 2024 •

edited

Loading

cpython-cla-bot bot commented Sep 26, 2024 •

edited

Loading

Eclips4 commented Sep 26, 2024 •

edited

Loading

Sep 27, 2024 •

edited

Loading