gh-104090: Update Resource Tracker to return exit code based on resource leaked found or not#106807

bityob · 2023-07-16T11:55:32Z

Add tests to catch the resource leak bug in collectedDurations and verified that it fails without the fix

Show output

$ ./python -m unittest test.test_concurrent_futures.FailingInitializerResourcesTest . /home/user/cpython/Lib/multiprocessing/resource_tracker.py:228: UserWarning: resource_tracker: There appear to be 3 leaked semaphore objects to clean up at shutdown warnings.warn('resource_tracker: There appear to be %d ' FTraceback (most recent call last): File "/home/user/cpython/Lib/multiprocessing/util.py", line 300, in _run_finalizers finalizer() File "/home/user/cpython/Lib/multiprocessing/util.py", line 224, in __call__ res = self._callback(*self._args, **self._kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/user/cpython/Lib/multiprocessing/synchronize.py", line 87, in _cleanup sem_unlink(name) FileNotFoundError: [Errno 2] No such file or directory ... ====================================================================== FAIL: test_forkserver (test.test_concurrent_futures.FailingInitializerResourcesTest.test_forkserver) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/user/cpython/Lib/test/test_concurrent_futures.py", line 318, in test_forkserver self._test(ProcessPoolForkserverFailingInitializerTest) File "/home/user/cpython/Lib/test/test_concurrent_futures.py", line 312, in _test self.assertEqual(_resource_tracker._exitcode, 0) AssertionError: 256 != 0 ====================================================================== FAIL: test_spawn (test.test_concurrent_futures.FailingInitializerResourcesTest.test_spawn) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/user/cpython/Lib/test/test_concurrent_futures.py", line 315, in test_spawn self._test(ProcessPoolSpawnFailingInitializerTest) File "/home/user/cpython/Lib/test/test_concurrent_futures.py", line 312, in _test self.assertEqual(_resource_tracker._exitcode, 0) AssertionError: 256 != 0 ---------------------------------------------------------------------- Ran 2 tests in 0.727s FAILED (failures=2) 0.33s

Add returning exit code from resource_tracker.py based on the cleanup result, so we can check what was the result
Add tests for Resource Tracker exit code based on leaked/no leaked resources

Issue: Leaked semaphore objects in test_concurrent_futures #104090

…r not and added tests for that

bedevere-bot · 2023-07-16T11:55:36Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Lib/multiprocessing/resource_tracker.py

Lib/test/_test_multiprocessing.py

…_concurrent_futures-v2

pitrou · 2023-07-17T16:24:42Z

Lib/test/_test_multiprocessing.py

+
+# Reset exit code value
+_resource_tracker._exitcode=None
+exit_code_assert=self.assertNotEqual


This is a bit confusing. Also, it will match any non-zero exit code which might not be desired (other problems may return some code not in {0, 1}). How about simply expected_exit_code = 0 if delete_queue else 1?

Hey,
I fixed the test and fixed the code too.
Since I saved the exit status (which was 256 when the exit code was 1),
so now the code saves the exit code and verifies it to 0/1
Thanks

Lib/test/_test_multiprocessing.py

pitrou · 2023-07-17T16:27:53Z

Lib/test/test_concurrent_futures.py

+runner=unittest.TextTestRunner()
+result=runner.run(test_class('test_initializer'))
+
+self.assertEqual(result.testsRun, 1)
+self.assertEqual(result.failures, [])
+self.assertEqual(result.errors, [])


Uhm, why not do the simple thing and add these checks somewhere in FailingInitializerMixin?
It should also avoid running those tests twice (which I assume this does).

The result object exists when we run the test using Test Runner, which is not the case on FailingInitializerMixin.
This is used for safety, ensuring the inner test passed before we check the resource tracker

Yes, but this is needlessly complex (and might interfere with other test options/customizations). Let's do the check at the end of said tests.

@pitrou I don't understand how you want those checks to be since we can check it only after a test run with the returned result, which is not the case in regular tests. Anyway, I'm removing those checks since it's not mandatory and just for safety

I honestly don't understand what the difficulty would be. You can make the resource tracker stop at the end of the test, or at the end of the testcase (in a teardown method for example).

… to convert from status code to exit code

…_concurrent_futures-v2

Lib/test/_test_multiprocessing.py

pitrou · 2023-07-18T10:00:41Z

Lib/test/_test_multiprocessing.py

+ Test the exit code of the resource tracker based on if there were left leaked resources when we stop the process.
+ If not leaked resources were found, exit code should be 0, otherwise 1


Thanks for the comment. Nit, but can you please ensure the line length remains reasonable (say < 80 characters) ? This can otherwise be annoying when reading/reviewing code.

Yes, I'll fix it. I'm used to 120 chars line length...

Misc/NEWS.d/next/Core and Builtins/2023-07-16-15-02-47.gh-issue-104090.oMjNa9.rst

Lib/multiprocessing/resource_tracker.py

Lib/test/_test_multiprocessing.py

…collect

…_concurrent_futures-v2

sunmy2019 · 2023-07-19T08:02:27Z

We should get #106795 merged first to pass the test.

pitrou · 2023-07-19T08:11:55Z

Lib/test/test_concurrent_futures.py

+runner=unittest.TextTestRunner()
+runner.run(test_class('test_initializer'))


Again, can you please instead put these checks directly in FailingInitializerMixin?
For example, add a tearDown method:
deftearDown(self): super().tearDown() # shutdown executorifself.mp_contextandself.mp_context.get_start_method() in ("spawn", "forkserver"): # Stop resource tracker manually now, so we can verify there are# not leaked resources by checking the process exit codefrommultiprocessing.resource_trackerimport_resource_tracker_resource_tracker._stop() self.assertEqual(_resource_tracker._exitcode, 0)

@pitrou Checking

Doesn't work
$ ./python -m unittest test.test_concurrent_futures.ProcessPoolForkserverFailingInitializerTest -v test_initializer (test.test_concurrent_futures.ProcessPoolForkserverFailingInitializerTest.test_initializer) ... /home/user/cpython/Lib/multiprocessing/resource_tracker.py:236: UserWarning: resource_tracker: There appear to be 3 leaked semaphore objects to clean up at shutdown warnings.warn('resource_tracker: There appear to be %d ' FAIL Traceback (most recent call last): File "/home/user/cpython/Lib/multiprocessing/util.py", line 300, in _run_finalizers finalizer() File "/home/user/cpython/Lib/multiprocessing/util.py", line 224, in __call__ res = self._callback(*self._args, **self._kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/user/cpython/Lib/multiprocessing/synchronize.py", line 87, in _cleanup sem_unlink(name) FileNotFoundError: [Errno 2] No such file or directory Traceback (most recent call last): File "/home/user/cpython/Lib/multiprocessing/util.py", line 300, in _run_finalizers finalizer() File "/home/user/cpython/Lib/multiprocessing/util.py", line 224, in __call__ res = self._callback(*self._args, **self._kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/user/cpython/Lib/multiprocessing/synchronize.py", line 87, in _cleanup sem_unlink(name) FileNotFoundError: [Errno 2] No such file or directory Traceback (most recent call last): File "/home/user/cpython/Lib/multiprocessing/util.py", line 300, in _run_finalizers finalizer() File "/home/user/cpython/Lib/multiprocessing/util.py", line 224, in __call__ res = self._callback(*self._args, **self._kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/user/cpython/Lib/multiprocessing/synchronize.py", line 87, in _cleanup sem_unlink(name) FileNotFoundError: [Errno 2] No such file or directory ====================================================================== FAIL: test_initializer (test.test_concurrent_futures.ProcessPoolForkserverFailingInitializerTest.test_initializer) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/user/cpython/Lib/test/test_concurrent_futures.py", line 298, in tearDown self.assertEqual(_resource_tracker._exitcode, 0) AssertionError: 1 != 0 ---------------------------------------------------------------------- Ran 1 test in 0.269s FAILED (failures=1)

Hmm, too bad :-(

…_concurrent_futures-v2

bityob · 2023-08-03T20:45:42Z

@pitrou
Hey,
Can you review again?
Thanks

pitrou · 2023-08-31T13:20:38Z

@bityob Sorry for the delay. There were some recent changes in git main that are causing conflicts. Could you merge from main and fix the conflicts? Thank you!

…est_concurrent_futures.py) to new location (Lib/test/test_concurrent_futures/test_init.py)

…_futures-v2' of github.com:bityob/cpython into pythongh-104090-fix-leaked-semaphors-on-test_concurrent_futures-v2

bityob · 2023-08-31T17:39:19Z

@bityob Sorry for the delay. There were some recent changes in git main that are causing conflicts. Could you merge from main and fix the conflicts? Thank you!

Hey @pitrou, Thank you!

I merged the main branch and resolved all conflicts.

Please review again

…_concurrent_futures-v2

pitrou

Thanks @bityob for the update. LGTM now, let's wait for CI.

pitrou · 2023-09-19T11:14:01Z

Unfortunately there are CI failures which look related. Can you take a look?

bityob · 2023-09-21T13:04:45Z

Unfortunately there are CI failures which look related. Can you take a look?

Yes, thanks. I'll check

…_concurrent_futures-v2

encukou · 2024-02-05T17:02:48Z

I'd love to be able to tell whether ResourceTracker was successful, but testing it on the “live” global _resource_tracker interferes with other tests (or regrtest).
I'm planning to send a PR based on this one, which will test a fresh ResourceTracker instance.

…urceTracker instance Add a new 'dummy' resource, which has no side-effects when cleaned up. The ResourceTracker test then registers this 'dummy', and either unregisters it or not.

encukou · 2024-02-13T15:05:54Z

My continuation of this PR is here: #115410

This builds on #106807, which adds a return code to ResourceTracker, to make future debugging easier. Testing this “in situ” proved difficult, since the global ResourceTracker is involved in test infrastructure. So, the tests here create a new instance and feed it fake data. --------- Co-authored-by: Yonatan Bitton <yonatan.bitton@perception-point.io> Co-authored-by: Yonatan Bitton <bityob@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org>

encukou · 2024-02-21T12:56:14Z

I've merged the continuation PR.
Thanks for doing the hard part!

…thonGH-115410) This builds on python#106807, which adds a return code to ResourceTracker, to make future debugging easier. Testing this “in situ” proved difficult, since the global ResourceTracker is involved in test infrastructure. So, the tests here create a new instance and feed it fake data. --------- Co-authored-by: Yonatan Bitton <yonatan.bitton@perception-point.io> Co-authored-by: Yonatan Bitton <bityob@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org>

Updated Resource Tracker to return exit code based on leaking found o…
70a35a2
…r not and added tests for that

bedevere-bot added the awaiting review label Jul 16, 2023

bedevere-bot mentioned this pull request Jul 16, 2023
Leaked semaphore objects in test_concurrent_futures #104090
Open

Add NEWS file
da7e245

bityob mentioned this pull request Jul 16, 2023
gh-104090: Fix unittest collectedDurations resources leak #106795
Merged

iritkatriel reviewed Jul 16, 2023
View reviewed changes

Lib/multiprocessing/resource_tracker.pyShow resolvedHide resolved

iritkatriel reviewed Jul 17, 2023
View reviewed changes

Lib/test/_test_multiprocessing.py Outdated Show resolvedHide resolved

bityob added 2 commits July 17, 2023 18:48

Use subTest for the parametrize the same tests with different values
1de022d

Merge branch 'main' intopythongh-104090-fix-leaked-semaphors-on-test…
48bea84
…_concurrent_futures-v2

iritkatriel requested a review from pitrou July 17, 2023 15:59

pitrou reviewed Jul 17, 2023
View reviewed changes

Lib/test/_test_multiprocessing.pyShow resolvedHide resolved

pitrou reviewed Jul 17, 2023
View reviewed changes

bityob added 4 commits July 18, 2023 12:03

Updated tests based on PR comments, and fix the resource_tracker code…
31bc4af
… to convert from status code to exit code

Clean test
47551a3

Added comment for test and simplified the logic
a02fdbe

Merge branch 'main' intopythongh-104090-fix-leaked-semaphors-on-test…
9d0369e
…_concurrent_futures-v2

pitrou reviewed Jul 18, 2023
View reviewed changes

Lib/test/_test_multiprocessing.pyShow resolvedHide resolved

pitrou reviewed Jul 18, 2023
View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2023-07-16-15-02-47.gh-issue-104090.oMjNa9.rst Outdated Show resolvedHide resolved

pitrou reviewed Jul 18, 2023
View reviewed changes

Lib/multiprocessing/resource_tracker.pyShow resolvedHide resolved

pitrou reviewed Jul 18, 2023
View reviewed changes

Lib/test/_test_multiprocessing.pyShow resolvedHide resolved

bityoband others added 4 commits July 18, 2023 13:38

Fix tests and News based on PR comments
4dbb777

Added reseting the exit code when the process should run again
bfd1e8e

Added more clearing queue methods beside del (closing and calling gc.…
043ae90
…collect

Merge branch 'main' intopythongh-104090-fix-leaked-semaphors-on-test…
850b5bb
…_concurrent_futures-v2

pitrou reviewed Jul 19, 2023
View reviewed changes

Merge branch 'main' intopythongh-104090-fix-leaked-semaphors-on-test…
6e82940
…_concurrent_futures-v2

Merge branch 'main' intopythongh-104090-fix-leaked-semaphors-on-test…
3b21d5c
…_concurrent_futures-v2

bityob requested a review from pitrou July 23, 2023 14:47

bityob added 3 commits August 31, 2023 20:32

Merge main
fbd18ec

Moved FailingInitializerResourcesTest class from old path (Lib/test/t…
e258cb3
…est_concurrent_futures.py) to new location (Lib/test/test_concurrent_futures/test_init.py)

Merge branch 'pythongh-104090-fix-leaked-semaphors-on-test_concurrent…
5b886bf
…_futures-v2' of github.com:bityob/cpython into pythongh-104090-fix-leaked-semaphors-on-test_concurrent_futures-v2

bityoband others added 2 commits August 31, 2023 20:46

Added missing import
915c8a3

Merge branch 'main' intopythongh-104090-fix-leaked-semaphors-on-test…
cd377e6
…_concurrent_futures-v2

pitrou changed the title ~~gh-104090: Updated Resource Tracker to return exit code based on resource leaked found or not~~gh-104090: Update Resource Tracker to return exit code based on resource leaked found or notSep 19, 2023

pitrou approved these changes Sep 19, 2023
View reviewed changes

bedevere-appbot added awaiting core review and removed awaiting review labels Sep 19, 2023

pitrou enabled auto-merge (squash) September 19, 2023 08:14

Merge branch 'main' intopythongh-104090-fix-leaked-semaphors-on-test…
27245ec
…_concurrent_futures-v2

bityob requested a review from gpshead as a code owner February 5, 2024 14:24

encukou mentioned this pull request Feb 13, 2024
gh-104090: Add exit code to multiprocessing ResourceTracker #115410
Merged

encukou closed this Feb 21, 2024

auto-merge was automatically disabled February 21, 2024 12:56
Pull request was closed

		Test the exit code of the resource tracker based on if there were left leaked resources when we stop the process.
		If not leaked resources were found, exit code should be 0, otherwise 1

		runner=unittest.TextTestRunner()
		runner.run(test_class('test_initializer'))

Uh oh!

gh-104090: Update Resource Tracker to return exit code based on resource leaked found or not#106807

gh-104090: Update Resource Tracker to return exit code based on resource leaked found or not #106807

Uh oh!

Conversation

bityob commented Jul 16, 2023• edited by pitrou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jul 16, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sunmy2019 commented Jul 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bityob commented Aug 3, 2023

Uh oh!

pitrou commented Aug 31, 2023

Uh oh!

bityob commented Aug 31, 2023

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Sep 19, 2023

Uh oh!

bityob commented Sep 21, 2023

Uh oh!

encukou commented Feb 5, 2024

Uh oh!

encukou commented Feb 13, 2024

Uh oh!

encukou commented Feb 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bityob commented Jul 16, 2023•
edited by pitrou
Loading