gh-108654: restore comprehension locals before handling exception #108659

carljm · 2023-08-30T00:55:39Z

If an inlined comprehension raises an exception, insert a cleanup handler to restore any locals' values that were overwritten within the comprehension, in case an outer exception handler uses those locals.

Issue: Comprehension iteration variable overwrite a variable of the same name in the outer scope inside a function with try..except #108654

Python/compile.c

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

bedevere-bot · 2023-08-30T14:14:27Z

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 8e8b41f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

Python/compile.c

carljm · 2023-08-30T17:26:19Z

In offline conversation, @markshannon and @gvanrossum suggested that we should just always do this cleanup (regardless of whether there is an outer handler or not), for consistency of what appears in the frame under traceback inspection. The cost is only in code size (no perf cost unless an exception is raised), but overall code size is still likely smaller than the non-inlined version with a separate code object, so that's not a big concern.

I will make that update.

I'll also look into the buildbot failures here and see if any of them are legit related to this PR.

* main: pythongh-108520: Fix bad fork detection in nested multiprocessing use case (python#108568) pythongh-108590: Revert pythongh-108657 (commit 400a1ce) (python#108686) pythongh-108494: Argument Clinic: Document how to generate code that uses the limited C API (python#108584) Document Python build requirements (python#108646) pythongh-101100: Fix Sphinx warnings in the Logging Cookbook (python#108678) Fix typo in multiprocessing docs (python#108666) pythongh-108669: unittest: Fix documentation for TestResult.collectedDurations (python#108670) pythongh-108590: Fix sqlite3.iterdump for invalid Unicode in TEXT columns (python#108657) Revert "pythongh-103224: Use the realpath of the Python executable in `test_venv` (pythonGH-103243)" (pythonGH-108667) pythongh-106320: Remove private _Py_ForgetReference() (python#108664) Mention Ellipsis pickling in the docs (python#103660) Revert "Use non alternate name for Kyiv (pythonGH-108533)" (pythonGH-108649) pythongh-108278: Deprecate passing the first param of sqlite3.Connection callback APIs by keyword (python#108632) pythongh-108455: peg_generator: install two stubs packages before running mypy (python#108637) pythongh-107801: Improve the accuracy of io.IOBase.seek docs (python#108268)

carljm · 2023-08-30T20:46:20Z

The refleak buildbots are all failing due to a refleak in test_import that was introduced a few days ago in main: #108696

carljm · 2023-08-30T21:06:14Z

This buildbot has been failing with the same crash for a while now:

s390x Fedora LTO + PGO PR

These Clang buildbots have all been seeing the same test_gdb failure (seems like #104736):

PPC64 Fedora PR has been seeing the same timeout in test_math for a while.

AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR has been failing test_socket for a while.

aarch64 Fedora Stable LTO + PGO PR and aarch64 Fedora Stable LTO PR have both been failing in test_concurrent_futures.test_shutdown multiple times already.

carljm · 2023-08-30T21:15:56Z

That leaves only the test_perf_profiler failure, seen only on AMD64 Arch Linux Asan Debug PR, and not seen recently on that builder otherwise. I am not able to reproduce this failure locally in a debug+asan build, even with running make buildbottest in the same way the worker does. I suspect this is a flaky failure; rebuilding to see if the issue reproduces.

carljm · 2023-08-30T23:30:45Z

Rebuild was successful, so I believe this PR is buildbot-clean, relative to main branch.

JelleZijlstra

Thanks for fixing this and thanks for chasing down all the buildbots.

miss-islington · 2023-08-30T23:50:53Z

Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

bedevere-bot · 2023-08-30T23:51:04Z

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

…on (pythonGH-108659) (cherry picked from commit d52c448) Co-authored-by: Carl Meyer <carl@oddbird.net> Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

bedevere-bot · 2023-08-31T00:34:28Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 Fedora Stable LTO 3.x has failed when building commit d52c448.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/336/builds/3864) and take a look at the build logs.
Check if the failure is related to this commit (d52c448) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/336/builds/3864

Failed tests:

test.test_concurrent_futures.test_shutdown

Failed subtests:

test_interpreter_shutdown - test.test_concurrent_futures.test_shutdown.ProcessPoolSpawnProcessPoolShutdownTest.test_interpreter_shutdown

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

447 tests OK.

10 slowest tests:

test_gdb: 2 min 59 sec
test.test_concurrent_futures.test_wait: 1 min 11 sec
test.test_multiprocessing_spawn.test_processes: 1 min 10 sec
test_signal: 1 min 3 sec
test_socket: 47.5 sec
test.test_multiprocessing_forkserver.test_processes: 41.4 sec
test_io: 38.0 sec
test_math: 37.1 sec
test_subprocess: 35.5 sec
test.test_multiprocessing_spawn.test_misc: 33.3 sec

1 test failed:
test.test_concurrent_futures.test_shutdown

14 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_ioctl
test_kqueue test_launcher test_startfile test_tkinter test_ttk
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test.test_concurrent_futures.test_shutdown

Total duration: 3 min 24 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/test_concurrent_futures/test_shutdown.py", line 49, in test_interpreter_shutdown
    self.assertFalse(err)
AssertionError: b'Exception in thread Thread-1:\nTraceback (most recent call last):\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 1059, in _bootstrap_inner\n    self.run()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 339, in run\n    self.add_call_item_to_queue()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 394, in add_call_item_to_queue\n    self.call_queue.put(_CallItem(work_id,\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 94, in put\n    self._start_thread()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 177, in _start_thread\n    self._thread.start()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 978, in start\n    _start_new_thread(self._bootstrap, ())\nRuntimeError: can\'t create new thread at interpreter shutdown\nTraceback (most recent call last):\n  File "<string>", line 1, in <module>\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/spawn.py", line 122, in spawn_main\n    exitcode = _main(fd, parent_sentinel)\n               ^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/spawn.py", line 132, in _main\n    self = reduction.pickle.load(from_parent)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/synchronize.py", line 115, in __setstate__\n    self._semlock = _multiprocessing.SemLock._rebuild(*state)\n                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\nFileNotFoundError: [Errno 2] No such file or directory\n' is not false

…ion (GH-108659) (#108700) gh-108654: restore comprehension locals before handling exception (GH-108659) (cherry picked from commit d52c448) Co-authored-by: Carl Meyer <carl@oddbird.net> Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

pythongh-108654: restore comprehension locals before handling exception

627ff93

bedevere-bot mentioned this pull request Aug 30, 2023

Comprehension iteration variable overwrite a variable of the same name in the outer scope inside a function with try..except #108654

Closed

2 tasks

corona10 reviewed Aug 30, 2023

View reviewed changes

Python/compile.c Outdated Show resolved Hide resolved

Python/compile.c Outdated Show resolved Hide resolved

carljm and others added 3 commits August 30, 2023 06:08

Apply suggestions from code review

c8eb6bf

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

fix location of POP_BLOCK

4c4eebc

no cleanup needed if no pushed locals

8e8b41f

carljm marked this pull request as ready for review August 30, 2023 13:40

carljm requested review from markshannon and iritkatriel as code owners August 30, 2023 13:40

bedevere-bot added the awaiting core review label Aug 30, 2023

carljm requested a review from JelleZijlstra August 30, 2023 13:41

JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2023

JelleZijlstra reviewed Aug 30, 2023

View reviewed changes

Python/compile.c Show resolved Hide resolved

carljm added 2 commits August 30, 2023 10:27

always restore comp-shadowed locals on exception

d49e911

JelleZijlstra approved these changes Aug 30, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 30, 2023

carljm added the needs backport to 3.12 only security fixes label Aug 30, 2023

carljm merged commit d52c448 into python:main Aug 30, 2023

bedevere-bot removed the awaiting merge label Aug 30, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 30, 2023

carljm deleted the icompexc branch August 30, 2023 23:51

carljm mentioned this pull request Sep 11, 2023

test_listcomps fails when run with -We #109182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-108654: restore comprehension locals before handling exception #108659

gh-108654: restore comprehension locals before handling exception #108659

carljm commented Aug 30, 2023 •

edited by bedevere-bot

Loading

bedevere-bot commented Aug 30, 2023

carljm commented Aug 30, 2023 •

edited

Loading

carljm commented Aug 30, 2023

carljm commented Aug 30, 2023

carljm commented Aug 30, 2023

carljm commented Aug 30, 2023

JelleZijlstra left a comment

miss-islington commented Aug 30, 2023

bedevere-bot commented Aug 30, 2023

bedevere-bot commented Aug 31, 2023

gh-108654: restore comprehension locals before handling exception #108659

gh-108654: restore comprehension locals before handling exception #108659

Conversation

carljm commented Aug 30, 2023 • edited by bedevere-bot Loading

bedevere-bot commented Aug 30, 2023

carljm commented Aug 30, 2023 • edited Loading

carljm commented Aug 30, 2023

carljm commented Aug 30, 2023

carljm commented Aug 30, 2023

carljm commented Aug 30, 2023

JelleZijlstra left a comment

Choose a reason for hiding this comment

miss-islington commented Aug 30, 2023

bedevere-bot commented Aug 30, 2023

bedevere-bot commented Aug 31, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

carljm commented Aug 30, 2023 •

edited by bedevere-bot

Loading

carljm commented Aug 30, 2023 •

edited

Loading