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-108654: restore comprehension locals before handling exception #108659

Merged
merged 6 commits into from Aug 30, 2023

Conversation

carljm
Copy link
Member

@carljm carljm commented Aug 30, 2023

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.

Python/compile.c Outdated Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
@carljm carljm marked this pull request as ready for review August 30, 2023 13:40
@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2023
@bedevere-bot
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 30, 2023
@carljm
Copy link
Member Author

carljm commented Aug 30, 2023

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
Copy link
Member Author

carljm commented Aug 30, 2023

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

@carljm
Copy link
Member Author

carljm commented Aug 30, 2023

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

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
Copy link
Member Author

carljm commented Aug 30, 2023

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
Copy link
Member Author

carljm commented Aug 30, 2023

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

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

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

@carljm carljm added the needs backport to 3.12 bug and security fixes label Aug 30, 2023
@carljm carljm merged commit d52c448 into python:main Aug 30, 2023
25 checks passed
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Aug 30, 2023
@carljm carljm deleted the icompexc branch August 30, 2023 23:51
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 30, 2023
…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
Copy link

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

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

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (d52c448) or if it is a false positive.
  5. 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

Yhg1s pushed a commit that referenced this pull request Aug 31, 2023
…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>
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.

None yet

5 participants