GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-104090: Update Resource Tracker to return exit code based on resource leaked found or not by bityob · Pull Request #106807 · python/cpython Conversation
|
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
||
| # Reset exit code value | ||
| _resource_tracker._exitcode = None | ||
| exit_code_assert = self.assertNotEqual |
There was a problem 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 simply expected_exit_code = 0 if delete_queue else 1?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 to 0/1
Thanks
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| 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.
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 in FailingInitializerMixin?
It should also avoid running those tests twice (which I assume this does).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 on FailingInitializerMixin.
This is used for safety, ensuring the inner test passed before we check the resource tracker
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
… to convert from status code to exit code
…_concurrent_futures-v2
There was an error while loading. Please reload this page.
| 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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...
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…_concurrent_futures-v2
|
We should get #106795 merged first to pass the test. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| runner = unittest.TextTestRunner() | ||
| runner.run(test_class('test_initializer')) |
There was a problem 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 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)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@pitrou Checking
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 -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)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hmm, too bad :-(
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…_concurrent_futures-v2
…_concurrent_futures-v2
|
@pitrou |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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
Hey @pitrou, Thank you! I merged the main branch and resolved all conflicts. Please review again |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…_concurrent_futures-v2
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Unfortunately there are CI failures which look related. Can you take a look? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Yes, thanks. I'll check |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…_concurrent_futures-v2
|
I'd love to be able to tell whether ResourceTracker was successful, but testing it on the “live” global |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
|
I've merged the continuation PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Pull request was closed
…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]>
…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]>
…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]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.