Skip to content

bpo-41994: Fix refcount issues in Python/import.c - #22632

Merged
encukou merged 10 commits into
python:masterfrom
serhiy-storchaka:import-refcounts
Jan 12, 2021
Merged

bpo-41994: Fix refcount issues in Python/import.c#22632
encukou merged 10 commits into
python:masterfrom
serhiy-storchaka:import-refcounts

Conversation

@serhiy-storchaka

@serhiy-storchakaserhiy-storchaka commented Oct 10, 2020•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@serhiy-storchakaserhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.9 labels Oct 10, 2020
pablogsal
pablogsal reviewed Oct 11, 2020
View reviewed changes
Comment threadPython/import.c Outdated
pablogsal
pablogsal reviewed Oct 11, 2020
View reviewed changes
Comment threadPython/import.c Outdated
pablogsal
pablogsal reviewed Oct 11, 2020
View reviewed changes
Comment threadPython/import.c Outdated
@pablogsalpablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 11, 2020
@bedevere-bot

bedevere-bot commented Oct 11, 2020

Copy link
Copy Markdown

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

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 11, 2020
@serhiy-storchaka

serhiy-storchaka commented Oct 22, 2020

Copy link
Copy Markdown
MemberAuthor

@ericsnowcurrently, please take a look. It has relation to the code added by you (support of arbitrary mapping as sys.modules).

@github-actions

github-actionsBot commented Dec 17, 2020

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Dec 17, 2020
encukou
encukou approved these changes Dec 29, 2020
View reviewed changes

@encukouencukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I went through the changes and they look OK to me!
To review, I needed to merge current master & resolve a small conflict; I hope you don't mind me pushing that into this PR.

@ericsnowcurrently, do you want to review as well? I might have missed some big-picture issue.

@serhiy-storchaka

serhiy-storchaka commented Dec 29, 2020

Copy link
Copy Markdown
MemberAuthor

Thank you @encukou. I planned to merge with master, but it needed some time because there is easy to miss some detail in large image when change how reference counted. Good opportunity to do one more review.

@serhiy-storchakaserhiy-storchaka added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 29, 2020
@bedevere-bot

bedevere-bot commented Dec 29, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 03e405f 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 29, 2020
@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Dec 30, 2020
@encukou

encukou commented Dec 30, 2020

Copy link
Copy Markdown
Member

Unfortunately that last merge was with a leaking master, which is now fixed. Another merge should fix the tests.

@serhiy-storchakaserhiy-storchaka added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 30, 2020
@bedevere-bot

bedevere-bot commented Dec 30, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit c31124f 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 30, 2020
@encukou

encukou commented Jan 5, 2021•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

And now the tests failed because of bpo-42794. This PR seems cursed :(

@bedevere-bot

bedevere-bot commented Jan 5, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @encukou for commit bc4ba0c 🤖

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

@encukouencukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 5, 2021
@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 5, 2021
@encukou
encukou merged commit 4db8988 into python:masterJan 12, 2021
@miss-islington

miss-islington commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

Thanks @serhiy-storchaka for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

Sorry, @serhiy-storchaka and @encukou, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4db8988420e0a122d617df741381b0c385af032c 3.9

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@serhiy-storchaka@adorilson
bpo-41994: Fix refcount issues in Python/import.c (pythonGH-22632)
a796d3e
https://bugs.python.org/issue41994
@ZeroIntensityZeroIntensity removed type-bug An unexpected behavior, bug, or error needs backport to 3.9 labels Feb 17, 2025
@serhiy-storchaka
serhiy-storchaka deleted the import-refcounts branch July 1, 2026 16:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannonbrettcannonbrettcannon approved these changes
@encukouencukouencukou approved these changes
@pablogsalpablogsalpablogsal approved these changes
@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrently
@ncoghlanncoghlanAwaiting requested review from ncoghlan
@warsawwarsawAwaiting requested review from warsaw

Assignees

@encukouencukou

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@serhiy-storchaka@bedevere-bot@encukou@miss-islington@brettcannon@pablogsal@the-knights-who-say-ni@ZeroIntensity