gh-156082: Do not run tests using the same machine-wide resource at once - #156902
gh-156082: Do not run tests using the same machine-wide resource at once#156902serhiy-storchaka wants to merge 3 commits into
Conversation
…e at once Two tests using the display, the sound device or the terminal at the same time interfere with each other. Add test.support.exclusive(), a decorator for a test method or a TestCase subclass, and test.support.requires_exclusive() for a whole test file. requires(), requires_resource() and bigmemtest() use them for such resources, so the existing tests need no change. Tests using different resources still run in parallel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documentation build overview
|
| try: | ||
| msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) | ||
| except OSError: | ||
| # LK_LOCK gives up after ten seconds, so retry ourselves. |
There was a problem hiding this comment.
This comment seems to be wrong/outdated. The code uses LK_NBLCK (NB stands for Non-Blocking), not LK_LOCK. The call returns immediately if the file region is already locked by another process.
| # The tests are not run in parallel, so there is nothing to exclude. | ||
| return None | ||
| try: | ||
| fd = os.open(os.path.join(directory, f'exclusive-{resource}.lock'), |
There was a problem hiding this comment.
Can you add 'python' or "python-test" to the filename? directory is the shared temporary directory if I understood correctly, so it's better to be more specific to help users to understand the purpose of this file.
| _lock_file(fd) | ||
| except OSError: | ||
| os.close(fd) | ||
| return None |
There was a problem hiding this comment.
Maybe add a catch-all "except: os.close(fd); raise" to make sure that the file descriptor is closed if something goes wrong. For example, if "import fcntl" and "import msvcrt" both raise an exception.
| f"waited for the disk", flush=True) | ||
| isolation._replay_test(self, payload, output, returncode) | ||
| # A real run allocates most of the memory of the machine. | ||
| with _exclusive_resource('memory'): |
There was a problem hiding this comment.
"bigmem" sounds more commonly used than "memory" for bigmem tests. You may add it to EXCLUSIVE_RESOURCES as well.
| yield | ||
| finally: | ||
| del _exclusive_locks[resource] | ||
| os.close(fd) |
There was a problem hiding this comment.
Yes, they are never deleted.
There was a problem hiding this comment.
I am not sure about this part. Should the locks be global for all users or per user?
Name the lock files, so that it is clear what left them in the temporary directory, and say why they are left. Close the file descriptor whatever goes wrong. Call the resource of bigmem tests 'bigmem'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lock file was created with mode 0o600, so only the user who created it could open it; the tests of other users were not serialized, silently. Create it readable by everybody and open it for reading: flock() locks such a file too. Windows needs a writable file descriptor, and has no O_NOFOLLOW. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I'm not sure that treating memory as an exclusive resource is a good idea. Currently, we only have a single "BigMem" buildbot worker running: aarch64 Ubuntu 24.04 BigMem 3.x. Its description says "Ubuntu 24.04.03 AArch64 big-memory buildbot (500GB). No builds between 10pm and 2am". It uses -j8 option to run 8 test worker processes in parallel. I'm not aware of memory usage issue on this buildbot. A build currently completes in 1 hour 32 min. If the total memory usage becomes an issue, it's possible to run less test worker processes in parallel. I'm worried that preventing bigmem tests from running in parallel would increase a lot the total time needed to run the Python test suite. If we want to limit the total memory usage and still run tests in parallel, we would need a more complicated logic. For example, count how much memory is currently used by test workers and pause a test which would allocate too much memory if it would exceed a limit. By the way, Python regrtest now reports the total memory usage of all test worker processes. On this aarch64 BigMem buildbot, the memory usage is betwen 23 MB and 657 MiB. Hum. These numbers look way lower than what I would expect from a "bigmem" test. Maybe the code to read the memory usage doesn't work well on this buildbot. I wrote code to track the memory usage to debug an issue where GitHub killed a CI job. In fact, Python allocated more than 15 GB of memory because of a bug in a test, and GitHub killed the container in this case. |
|
For the buildbot, running tests with But this can save these who run all tests, including bigmem tests, in parallel on less monstrous machine. Only tiny fractions of tests are bigmem tests, so in most cases they will not conflict, but in rare cases they can kill the machine. Anyway, supporting bigmem tests costs nothing if the mechanism already exists. |
Two tests using the display, the sound device or the terminal at the same time interfere with each other. Running the GUI tests in parallel fails 100% of the time on Windows and about half the time on Linux.
Add
test.support.exclusive(resource), a decorator for a test method or aTestCasesubclass, andtest.support.requires_exclusive(resource)for a whole test file:requires(),requires_resource()andbigmemtest()(only for a real run) claim the resource themselves, so no existing test needs a change. The resources treated as machine-wide areaudio,console,cursesandgui, plusmemoryfor bigmem tests.Each resource is a lock file in the directory which the test runner names in an environment variable. It sets it only when it runs more than one worker process, so a sequential run,
-j1andpython -m unittestlock nothing. The lock is released by closing the file, so a test which crashes or is killed does not keep it.Tests using different resources still run in parallel; only tests using the same one wait for each other. They therefore no longer overlap:
test_tkinter,test_ttkandtest_idlewith-j4take 7.1 seconds instead of 3.5, which is the sum of their durations instead of the largest of them.🤖 Generated with Claude Code