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-101747: Fix refleak in new OrderedDict repr #101748

Merged
merged 2 commits into from
Feb 10, 2023
Merged

gh-101747: Fix refleak in new OrderedDict repr #101748

merged 2 commits into from
Feb 10, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 9, 2023 •
edited by bedevere-bot
Loading

After: ./python.exe -m test -v test_ordered_dict -R 3:3

Ran 279 tests in 1.460s

OK
.

== Tests result: SUCCESS ==

1 test OK.

Total duration: 10.2 sec
Tests result: SUCCESS

@sobolevn sobolevn requested a review from corona10 February 9, 2023 17:10
@bedevere-bot bedevere-bot mentioned this pull request Feb 9, 2023
Closed
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 9, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit f8adfe9 🤖

If you want to schedule another build, you need to add the :hammer: 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 Feb 9, 2023
@erlend-aasland
Copy link
Contributor

The aarch64 RHEL8 refleak bot still reports a leak. Could you please examine, @sobolevn?

serhiy-storchaka
serhiy-storchaka reviewed Feb 9, 2023
View reviewed changes
Objects/odictobject.c Outdated
@@ -1388,6 +1388,7 @@ odict_repr(PyODictObject *self)

Done:
Py_ReprLeave((PyObject *)self);
Py_XDECREF(dcopy);
Copy link
Member

Choose a reason for hiding this comment

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

You can move it above the Done label and change Py_XDECREF to Py_DECREF.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, it might be a slightly more readable! I will wait for all tests to finish and apply it :)

@sobolevn
Copy link
Member Author

sobolevn commented Feb 9, 2023

@erlend-aasland it reports:

test_importlib leaked [6, 4, 2] references, sum=12
test_importlib leaked [4, 4, 2] memory blocks, sum=10

I think these leaks are not related 🤔

Should I fix it here?

@erlend-aasland
Copy link
Contributor

Should I fix it here?

I prefer one fix per PR; that makes it easier to bisect in case something goes wrong, and it also makes for a cleaner git history.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 9, 2023

@erlend-aasland then I consider this done:

  1. Review suggestion is applied
  2. New issue is created: Refleak in test_importlib on aarch64 RHEL8 #101766

corona10
corona10 approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10
Copy link
Member

corona10 commented Feb 9, 2023

I will merge this PR after waiting for @serhiy-storchaka approval

test_importlib leaked [6, 4, 2] references, sum=12

I can not reproduce the issue on my macOS. It sounds like we have to debug on the RHEL machine.

@sobolevn sobolevn mentioned this pull request Feb 9, 2023
Closed
@rhettinger rhettinger merged commit 34c50ce into python:main Feb 10, 2023
carljm added a commit to carljm/cpython that referenced this pull request Feb 10, 2023
@carljm
Merge branch 'main' into inlinecomp2
add772e 
* main:
  Fix some typos in asdl_c.py (pythonGH-101757)
  pythongh-101747: Fix refleak in new `OrderedDict` repr (pythonGH-101748)
  pythongh-101430: Update tracemalloc to handle presize properly. (pythongh-101745)
  pythonGH-101228: Fix typo in docstring for read method of `_io.TextIOWrapper` class (python#101227)
  Fix typo in `test_fstring.py` (python#101600)
  pythongh-101726: Update the OpenSSL version to 1.1.1t (pythonGH-101727)
  pythongh-101283: Fix 'versionchanged' for the shell=True fallback on Windows in 3.12 (pythonGH-101728)
  LibFFI build requires x64 Cygwin, and skip the ARM build (pythonGH-101743)
@arhadthedev arhadthedev mentioned this pull request Feb 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@corona10 corona10 corona10 approved these changes

@rhettinger rhettinger rhettinger approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sobolevn @bedevere-bot @erlend-aasland @corona10 @rhettinger @serhiy-storchaka @kumaraditya303