-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-104090: Update Resource Tracker to return exit code based on resource leaked found or not #106807
Changes from 4 commits
70a35a2
da7e245
1de022d
48bea84
31bc4af
47551a3
a02fdbe
9d0369e
4dbb777
bfd1e8e
043ae90
850b5bb
6e82940
a5cb934
ccd736c
3b21d5c
fbd18ec
e258cb3
5b886bf
915c8a3
cd377e6
27245ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5536,6 +5536,37 @@ def test_too_long_name_resource(self): | |
with self.assertRaises(ValueError): | ||
resource_tracker.register(too_long_name_resource, rtype) | ||
|
||
def _test_resource_tracker_leak_resources(self, context, delete_queue): | ||
from multiprocessing.resource_tracker import _resource_tracker | ||
_resource_tracker.ensure_running() | ||
self.assertTrue(_resource_tracker._check_alive()) | ||
|
||
# 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey, |
||
|
||
mp_context = multiprocessing.get_context(context) | ||
|
||
# Keep it on variable, so it won't be cleared yet | ||
q = mp_context.Queue() | ||
if delete_queue: | ||
del q | ||
sunmy2019 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
exit_code_assert = self.assertEqual | ||
|
||
self.assertIsNone(_resource_tracker._exitcode) | ||
_resource_tracker._stop() | ||
|
||
exit_code_assert(_resource_tracker._exitcode, 0) | ||
|
||
def test_resource_tracker_exit_code(self): | ||
sunmy2019 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for context in ["spawn", "forkserver"]: | ||
pitrou marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for delete_queue in [True, False]: | ||
with self.subTest(context=context, delete_queue=delete_queue): | ||
self._test_resource_tracker_leak_resources( | ||
context=context, | ||
delete_queue=delete_queue, | ||
) | ||
|
||
|
||
class TestSimpleQueue(unittest.TestCase): | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -290,6 +290,35 @@ def _assert_logged(self, msg): | |
create_executor_tests(FailingInitializerMixin) | ||
|
||
|
||
@unittest.skipIf(sys.platform == "win32", "Resource Tracker doesn't run on Windows") | ||
class FailingInitializerResourcesTest(unittest.TestCase): | ||
""" | ||
Source: https://github.com/python/cpython/issues/104090 | ||
""" | ||
|
||
def _test(self, test_class): | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Uhm, why not do the simple thing and add these checks somewhere in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 on There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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). |
||
|
||
# GH-104090: | ||
# 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) | ||
|
||
def test_spawn(self): | ||
self._test(ProcessPoolSpawnFailingInitializerTest) | ||
|
||
def test_forkserver(self): | ||
self._test(ProcessPoolForkserverFailingInitializerTest) | ||
|
||
|
||
class ExecutorShutdownTest: | ||
def test_run_after_shutdown(self): | ||
self.executor.shutdown() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Updated Resource Tracker to return exit code based on resource leaked found | ||
or not | ||
sunmy2019 marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.