Skip to content

gh-101766: Fix refleak for _BlockingOnManager resources#101942

Merged
corona10 merged 2 commits into
python:mainfrom
corona10:gh-101766
Feb 17, 2023
Merged

gh-101766: Fix refleak for _BlockingOnManager resources#101942
corona10 merged 2 commits into
python:mainfrom
corona10:gh-101766

Conversation

@corona10
Copy link
Copy Markdown
Member

@corona10 corona10 commented Feb 16, 2023 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot mentioned this pull request Feb 16, 2023
Closed
@corona10 corona10 added skip news 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Feb 16, 2023
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Feb 16, 2023

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

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 16, 2023
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 16, 2023
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Feb 16, 2023

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

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 16, 2023
gvanrossum
gvanrossum approved these changes Feb 16, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM, though I never saw this code before.

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Feb 16, 2023 •
edited
Loading

The current failure does not relate to this PR..
Also, it is failed with the main branch too: eb0c485 and also for 89ac665

$> ./python -m test test_imp -R 3:3 -v

======================================================================
FAIL: test_singlephase_variants (test.test_imp.ImportTests.test_singlephase_variants) [_testsinglephase]
Exercise the most meaningful variants described in Python/import.c.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cpython/Lib/test/test_imp.py", line 366, in test_singlephase_variants
    self.assertEqual(init_count, expected_init_count)
AssertionError: 3 != 1

======================================================================
FAIL: test_singlephase_variants (test.test_imp.ImportTests.test_singlephase_variants) [_testsinglephase_basic_wrapper]
Exercise the most meaningful variants described in Python/import.c.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cpython/Lib/test/test_imp.py", line 383, in test_singlephase_variants
    self.assertEqual(init_count, expected_init_count)
AssertionError: 3 != 2

----------------------------------------------------------------------
Ran 30 tests in 0.106s

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Feb 16, 2023

@ericsnowcurrently Can you please take a look?

sobolevn
sobolevn approved these changes Feb 16, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

We can safely remove it, because it is reset here: self.blocked_on = _blocking_on.setdefault(self.thread_id, [])

Thanks for fixing this!

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Feb 16, 2023

@gvanrossum @sobolevn
I will merge this PR after I check the leak test is passed.