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-99729: Unlink frames before clearing them #100030

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Dec 5, 2022

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 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 brandtbucher self-assigned this Dec 5, 2022
@brandtbucher brandtbucher changed the title Frame teardown GH-99729: Unlink frames before clearing them Dec 5, 2022
@brandtbucher
Copy link
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 pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 5, 2022
@bedevere-bot
Copy link

🤖 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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 5, 2022
@pablogsal
Copy link
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.

@brandtbucher
Copy link
Member Author

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

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

🤖 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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 5, 2022
@markshannon
Copy link
Member

Looks good. I think the buildbot failures are unrelated.

@pablogsal pablogsal merged commit b72014c into python:main Dec 6, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
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

@pablogsal
Copy link
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

@brandtbucher
Copy link
Member Author

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

I opened it yesterday: #100047

@hauntsaninja hauntsaninja removed the needs backport to 3.11 only security fixes label Mar 26, 2023
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