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:

def tearDown(self): super().tearDown() # shutdown executor if self.mp_context and self.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 code from multiprocessing.resource_tracker import _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 <[email protected]> Co-authored-by: Yonatan Bitton <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]>

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 <[email protected]> Co-authored-by: Yonatan Bitton <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]>

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.py Show resolved Hide resolved

iritkatriel reviewed Jul 17, 2023

View reviewed changes

Lib/test/_test_multiprocessing.py Outdated Show resolved Hide 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' into pythongh-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.py Show resolved Hide 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' into pythongh-104090-fix-leaked-semaphors-on-test…

9d0369e

…_concurrent_futures-v2

pitrou reviewed Jul 18, 2023

View reviewed changes

Lib/test/_test_multiprocessing.py Show resolved Hide 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 resolved Hide resolved

pitrou reviewed Jul 18, 2023

View reviewed changes

Lib/multiprocessing/resource_tracker.py Show resolved Hide resolved

pitrou reviewed Jul 18, 2023

View reviewed changes

Lib/test/_test_multiprocessing.py Show resolved Hide resolved

bityob and 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' into pythongh-104090-fix-leaked-semaphors-on-test…

850b5bb

…_concurrent_futures-v2

pitrou reviewed Jul 19, 2023

View reviewed changes

Merge branch 'main' into pythongh-104090-fix-leaked-semaphors-on-test…

6e82940

…_concurrent_futures-v2

bityob added 2 commits July 20, 2023 12:56

Merge branch 'main' into pythongh-104090-fix-leaked-semaphors-on-test…

ccd736c

…_concurrent_futures-v2

Merge branch 'main' into pythongh-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

bityob and others added 2 commits August 31, 2023 20:46

Added missing import

915c8a3

Merge branch 'main' into pythongh-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 not Sep 19, 2023

pitrou approved these changes Sep 19, 2023

View reviewed changes

bedevere-app bot 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' into pythongh-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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

bityob commented Jul 16, 2023 •

edited by pitrou

Loading

bedevere-bot commented Jul 16, 2023

pitrou Jul 17, 2023

bityob Jul 18, 2023

pitrou Jul 17, 2023

bityob Jul 18, 2023

pitrou Jul 18, 2023

bityob Jul 18, 2023

pitrou Jul 18, 2023

pitrou Jul 18, 2023

bityob Jul 18, 2023

sunmy2019 commented Jul 19, 2023

pitrou Jul 19, 2023

bityob Jul 19, 2023

bityob Jul 19, 2023

pitrou Jul 19, 2023

bityob commented Aug 3, 2023

pitrou commented Aug 31, 2023

bityob commented Aug 31, 2023

pitrou left a comment

pitrou commented Sep 19, 2023

bityob commented Sep 21, 2023

encukou commented Feb 5, 2024

encukou commented Feb 13, 2024

encukou commented Feb 21, 2024

		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'))

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

Conversation

bityob commented Jul 16, 2023 • edited by pitrou Loading

bedevere-bot commented Jul 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunmy2019 commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bityob commented Aug 3, 2023

pitrou commented Aug 31, 2023

bityob commented Aug 31, 2023

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Sep 19, 2023

bityob commented Sep 21, 2023

encukou commented Feb 5, 2024

encukou commented Feb 13, 2024

encukou commented Feb 21, 2024

bityob commented Jul 16, 2023 •

edited by pitrou

Loading