gh-104090: Update Resource Tracker to return exit code based on resource leaked found or not by bityob · Pull Request #106807 · python/cpython · GitHub

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

Most changes to Python require a NEWS entry.

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

…_concurrent_futures-v2

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

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

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...

…collect

…_concurrent_futures-v2

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

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

@pitrou
Hey,
Can you review again?
Thanks

@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 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

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

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

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

Yes, thanks. I'll check

…_concurrent_futures-v2

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.

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>

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

Comment thread Lib/multiprocessing/resource_tracker.py

iritkatriel reviewed Jul 17, 2023

View reviewed changes

Comment thread Lib/test/_test_multiprocessing.py Outdated

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

Comment thread Lib/test/_test_multiprocessing.py

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

Comment thread Lib/test/_test_multiprocessing.py

pitrou reviewed Jul 18, 2023

View reviewed changes

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

pitrou reviewed Jul 18, 2023

View reviewed changes

Comment thread Lib/multiprocessing/resource_tracker.py

pitrou reviewed Jul 18, 2023

View reviewed changes

Comment thread Lib/test/_test_multiprocessing.py

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

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

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

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

		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 by bityob · Pull Request #106807 · python/cpython · GitHub

Repository navigation

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bityob commented Jul 16, 2023 •
edited by pitrou
Loading