Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-104090: Update Resource Tracker to return exit code based on resource leaked found or not#106807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…r not and added tests for that
bedevere-bot commentedJul 16, 2023
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Lib/test/_test_multiprocessing.py Outdated
| # Reset exit code value | ||
| _resource_tracker._exitcode = None | ||
| exit_code_assert = self.assertNotEqual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 simplyexpected_exit_code = 0 if delete_queue else 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to0/1
Thanks
Uh oh!
There was an error while loading.Please reload this page.
Lib/test/test_concurrent_futures.py Outdated
| runner = unittest.TextTestRunner() | ||
| result = runner.run(test_class('test_initializer')) | ||
| self.assertEqual(result.testsRun, 1) | ||
| self.assertEqual(result.failures, []) | ||
| self.assertEqual(result.errors, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Uhm, why not do the simple thing and add these checks somewhere inFailingInitializerMixin?
It should also avoid running those tests twice (which I assume this does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The result object exists when we run the test using Test Runner, which is not the case onFailingInitializerMixin.
This is used for safety, ensuring the inner test passed before we check the resource tracker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
Uh oh!
There was an error while loading.Please reload this page.
Lib/test/_test_multiprocessing.py Outdated
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
We should get#106795 merged first to pass the test. |
Lib/test/test_concurrent_futures.py Outdated
| runner = unittest.TextTestRunner() | ||
| runner.run(test_class('test_initializer')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Again, can you please instead put these checks directly inFailingInitializerMixin?
For example, add atearDown 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@pitrou Checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Doesn't work
$ ./python -m unittest test.test_concurrent_futures.ProcessPoolForkserverFailingInitializerTest -vtest_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 'FAILTraceback (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 directoryTraceback (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 directoryTraceback (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.269sFAILED (failures=1)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm, too bad :-(
…_concurrent_futures-v2
…_concurrent_futures-v2
@pitrou |
@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 intopythongh-104090-fix-leaked-semaphors-on-test_concurrent_futures-v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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? |
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 |
…urceTracker instanceAdd a new 'dummy' resource, which has no side-effects whencleaned up.The ResourceTracker test then registers this 'dummy', and eitherunregisters it or not.
My continuation of this PR is here:#115410 |
This builds on#106807, which addsa return code to ResourceTracker, to make future debugging easier.Testing this “in situ” proved difficult, since the global ResourceTracker isinvolved in test infrastructure. So, the tests here create a new instance andfeed 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. |
Pull request was closed
…thonGH-115410)This builds onpython#106807, which addsa return code to ResourceTracker, to make future debugging easier.Testing this “in situ” proved difficult, since the global ResourceTracker isinvolved in test infrastructure. So, the tests here create a new instance andfeed 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>
…thonGH-115410)This builds onpython#106807, which addsa return code to ResourceTracker, to make future debugging easier.Testing this “in situ” proved difficult, since the global ResourceTracker isinvolved in test infrastructure. So, the tests here create a new instance andfeed 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>
…thonGH-115410)This builds onpython#106807, which addsa return code to ResourceTracker, to make future debugging easier.Testing this “in situ” proved difficult, since the global ResourceTracker isinvolved in test infrastructure. So, the tests here create a new instance andfeed 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>
Uh oh!
There was an error while loading.Please reload this page.
collectedDurationsand verified that it fails without the fixShow output
resource_tracker.pybased on the cleanup result, so we can check what was the result