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.

Sign up for GitHub

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

Jump to bottom

GH-94857: fix test_io refleak #94858

Merged
merged 6 commits into from
Jul 18, 2022
Merged

GH-94857: fix test_io refleak #94858

merged 6 commits into from
Jul 18, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jul 14, 2022 •
edited by miss-islington
Loading

@kumaraditya303 kumaraditya303 mentioned this pull request Jul 14, 2022
Closed
@kumaraditya303 kumaraditya303 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 14, 2022
@bedevere-bot
Copy link

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

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 Jul 14, 2022
@kumaraditya303 kumaraditya303 added the needs backport to 3.11 only security fixes label Jul 14, 2022
@kumaraditya303 kumaraditya303 self-assigned this Jul 14, 2022
tiran
tiran requested changes Jul 14, 2022
View reviewed changes
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

There are more possible refleaks in error paths. It would be better to incref encodings eagerly and decref it in error paths.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka
serhiy-storchaka reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

There are still leaks in three return statements between lines 1260 and 1280.

@serhiy-storchaka
Copy link
Member

Concur with @tiran.

@kumaraditya303
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from tiran July 15, 2022 08:16
@kumaraditya303 kumaraditya303 mentioned this pull request Jul 15, 2022
Merged
tiran
tiran approved these changes Jul 18, 2022
View reviewed changes
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@miss-islington miss-islington merged commit 631160c into python:main Jul 18, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry @kumaraditya303, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 631160c262b40bf4ce3da6cd7bbb972ae2e9fc91 3.11

@kumaraditya303 kumaraditya303 added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Jul 18, 2022
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 18, 2022
@kumaraditya303 @miss-islington
pythonGH-94857: fix test_io refleak (pythonGH-94858)
6d2b36c 
(cherry picked from commit 631160c)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot
Copy link

GH-94959 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 18, 2022
miss-islington added a commit that referenced this pull request Jul 18, 2022
@miss-islington @kumaraditya303
GH-94857: fix test_io refleak (GH-94858)
a914fa9 
(cherry picked from commit 631160c)

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
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@tiran tiran tiran approved these changes

Assignees

@miss-islington miss-islington

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kumaraditya303 @bedevere-bot @serhiy-storchaka @miss-islington @tiran