Skip to content

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn commented Sep 29, 2024

The leak was in setting _testcapi.set_nomemory(0, 1), in other places it is only used together with assert_python_ok

I also marked this test as cpython_only and I also skip it if _testcapi is not present.

Local run:

» ./python.exe -m test test_class -m test_detach_materialized_dict_no_memory -R 3:3 Using random seed: 365282797 0:00:00 load avg: 1.55 Run 1 test sequentially in a single process 0:00:00 load avg: 1.55 [1/1] test_class beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more) 123:456 XX. ... == Tests result: SUCCESS == 1 test OK. Total duration: 403 ms Total tests: run=1 (filtered) Total test files: run=1/1 (filtered) Result: SUCCESS 

Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

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

I think this looks fine, but I'm worried about what problems that _testcapi.set_nomemory is causing. Could you elaborate on that? (This seems like more of a bandaid than an actual fix for that function.)

Copy link
Member

@Eclips4Eclips4 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 thought of the same approach.

@sobolevn
Copy link
MemberAuthor

@ZeroIntensity please, take a look at fm_setup_hooks, probably something there is not cleaned up.

The more complex fix would be:

  • make sure that fm_remove_hooks completely disables all modifications done in fm_setup_hooks
  • create a context manager to automatically clean up this context in tests

But, this change would be much more complex, that's why I went with the simplier (and already existing) fix instead.

@ZeroIntensity
Copy link
Member

Yeah, it's good to get this in sooner. I'll look into fixing set_nomemory, but it's odd because as far as I can see fm_setup_hooks makes no additional allocations, so I'm not exactly sure what could be getting leaked.

@zware
Copy link
Member

!buildbot .RHEL8 Refleak.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zware for commit e7a409b 🤖

The command will test the builders whose names match following regular expression: .*RHEL8 Refleak.*

The builders matched are:

  • s390x RHEL8 Refleaks PR
  • AMD64 RHEL8 Refleaks PR
  • aarch64 RHEL8 Refleaks PR
  • PPC64LE RHEL8 Refleaks PR

@zware
Copy link
Member

The buildbots look happy with this, so I'm going ahead with the merge to get them cleaned up. @markshannon, it could still use a post-merge OK from you to confirm that it's still testing what you wanted to test.

@zwarezware merged commit 6f4d64b into python:mainSep 30, 2024
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 30, 2024
@bedevere-app
Copy link

GH-124777 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Sep 30, 2024
Yhg1s pushed a commit that referenced this pull request Sep 30, 2024
…y` (GH-124769) (#124777) gh-124722: Fix leak in `test_detach_materialized_dict_no_memory` (GH-124769) (cherry picked from commit 6f4d64b) Co-authored-by: sobolevn <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sobolevn@ZeroIntensity@zware@bedevere-bot@Eclips4