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-103092: Isolate _collections #103093

Merged
merged 22 commits into from
Apr 12, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Mar 28, 2023

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM, I finished the remaining bits.

@kumaraditya303
Copy link
Contributor

Now it crashes in the dict watcher deallocation, cc @carljm

@erlend-aasland erlend-aasland marked this pull request as ready for review April 9, 2023 20:01
@erlend-aasland erlend-aasland requested a review from carljm April 9, 2023 20:02
@erlend-aasland
Copy link
Contributor Author

Now it crashes in the dict watcher deallocation, cc @carljm

See 45a26f3, which I believe is the correct prescription. Notice that PyDict_Type.tp_init is called explicitly from defdict_init. IIRC, that is not needed, but I may be wrong (I'm unable to check that right now).

@erlend-aasland erlend-aasland requested review from kumaraditya303 and removed request for carljm April 9, 2023 22:41
@erlend-aasland

This comment was marked as outdated.

@erlend-aasland
Copy link
Contributor Author

BTW, there are ref. leaks, so we need to hold off a little bit more. Try for example checking ref. leaks on test_defaultdict.

@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 10, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 0dcfcef 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 10, 2023
@rhettinger rhettinger removed their request for review April 11, 2023 17:44
@kumaraditya303 kumaraditya303 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 0381b45 🤖

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 Apr 12, 2023
@erlend-aasland
Copy link
Contributor Author

Good find! No ref.leaks locally over here.

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

Final approval!

@erlend-aasland
Copy link
Contributor Author

Good job!

@kumaraditya303 kumaraditya303 merged commit 52f96d3 into python:main Apr 12, 2023
@kumaraditya303 kumaraditya303 deleted the isolate-collections branch April 12, 2023 12:51
aisk pushed a commit to aisk/cpython that referenced this pull request Apr 18, 2023
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.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.

4 participants