FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-99729: Unlink frames before clearing them by brandtbucher · Pull Request #100030 · python/cpython · GitHub

/ cpython Public

GH-99729: Unlink frames before clearing them - #100030

Merged
pablogsal merged 4 commits into
python:mainfrom
brandtbucher:frame-teardown
Dec 6, 2022
Merged

GH-99729: Unlink frames before clearing them#100030
pablogsal merged 4 commits into
python:mainfrom
brandtbucher:frame-teardown

Conversation

brandtbucher commented Dec 5, 2022
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Thanks for your patience with this.

I decided to switch strategies mid-fix after a discussion with @markshannon this morning: instead of hacking the frame to look "incomplete" just before teardown, I think unlinking the frame before clearing it (instead of after) is a better approach that is easier to reason about and keeps extra code out of the hot path when popping frames.

This is going to need a manual backport to 3.11. I'll start on it now to hopefully unblock releases.

CC: @pablogsal

brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump needs backport to 3.11 only security fixes labels Dec 5, 2022
brandtbucher self-assigned this Dec 5, 2022
brandtbucher changed the title Frame teardown GH-99729: Unlink frames before clearing them Dec 5, 2022

Copy link
Copy Markdown
Member Author

Not sure if we want buildbots on this (normally I would run them, but I also don't want to further delay the releases with a bunch of extra CI checks). I'm confident in the fix, though.

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 5, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 6210626 🤖

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

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 5, 2022

Copy link
Copy Markdown
Member

Not sure if we want buildbots on this (normally I would run them, but I also don't want to further delay the releases with a bunch of extra CI checks). I'm confident in the fix, though.

Thanks a lot for fixing this ❤️

I am executing the buildbots at least to see the quick ones to double check there is nothing unexpected. I will manually test Refleaks after before the release.

Copy link
Copy Markdown
Member Author

Hm, looks like we need to skip the new test on webassembly builds.

brandtbucher added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 5, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit af155c7 🤖

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

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 5, 2022

Copy link
Copy Markdown
Member

Looks good. I think the buildbot failures are unrelated.

pablogsal merged commit b72014c into python:main Dec 6, 2022

Copy link
Copy Markdown
Contributor

Thanks @brandtbucher for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

Copy link
Copy Markdown
Contributor

Sorry, @brandtbucher and @pablogsal, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b72014c783e5698beb18ee1249597e510b8bcb5a 3.11

Copy link
Copy Markdown
Member

Thanks for fixing this @brandtbucher! ❤️

This is going to need a manual backport to 3.11. I'll start on it now to hopefully unblock releases.

Please, ping me as soon as you have the manual backport ready

Copy link
Copy Markdown
Member Author

Please, ping me as soon as you have the manual backport ready

I opened it yesterday: #100047

hauntsaninja removed the needs backport to 3.11 only security fixes label Mar 26, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL