gh-109461: Update logging module lock acquisition to use context manager#109462
Conversation
…t manager so that it is safer and easier to use.
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
…ock-acquisition' into pythongh-109461-update-logging-lock-acquisition
…text manager, or a null context.
…ock-acquisition' into pythongh-109461-update-logging-lock-acquisition
…ock-acquisition' into pythongh-109461-update-logging-lock-acquisition
… bool(_lock) always returns true
I'm away for a couple of weeks (with sporadic Internet access) and have set off tests in the buildbot fleet, with some results still to come in. If all the failures are unrelated to this change, I think we can merge this PR. |
Sadly, there are many unstable tests these days, I'm trying to fix most of them. I mean: most failures are unrelated to this PR. But someone has to check. Or later, you can rebase the PR on the main branch, and schedule a new buildbot job. |
|
🤖 New build scheduled with the buildbot fleet by @AA-Turner for commit 797ae0d 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
Ubuntu reports |
There are many unstable tests. All "FAILURE then SUCCESS" are unstable tests. They are unrelated to this change and you can ignore them. You can browse into my issues https://github.com/python/cpython/issues/vstinner to see recent unstable tests. |
|
10 buildbots failed:
None seem logging related, so seems safe to merge. A |
|
@vsajip, maintainer of logging wrote:
In short, he approved the PR. |
|
Merged. Thanks @dcollison, it's a nice cleanup. It may make the logging safer, since Example in Python 3.10, see also issue gh-74225: if (_Py_atomic_load_relaxed(eval_breaker)) {
opcode = _Py_OPCODE(*next_instr);
if (opcode != SETUP_FINALLY &&
opcode != SETUP_WITH &&
opcode != BEFORE_ASYNC_WITH &&
opcode != YIELD_FROM) {
/* Few cases where we skip running signal handlers and other
pending calls:
- If we're about to enter the 'with:'. It will prevent
emitting a resource warning in the common idiom
'with open(path) as file:'.
- If we're about to enter the 'async with:'.
- If we're about to enter the 'try:' of a try/finally (not
*very* useful, but might help in some cases and it's
traditional)
- If we're resuming a chain of nested 'yield from' or
'await' calls, then each frame is parked with YIELD_FROM
as its next opcode. If the user hit control-C we want to
wait until we've reached the innermost frame before
running the signal handler and raising KeyboardInterrupt
(see bpo-30039).
*/
if (eval_frame_handle_pending(tstate) != 0) {
goto error;
}
}
} |
|
|
…ython#109462) Co-authored-by: Victor Stinner <vstinner@python.org>
…ython#109462) Co-authored-by: Victor Stinner <vstinner@python.org>
python/cpython#109462 removed `logging._acquireLock`. Fortunately, python/cpython#3385 simplified lock creation so that `with logging._lock:` is sufficient; that PR made it into Python 3.7, which is already pastescript's minimum Python requirement.
python/cpython#109462 removed `logging._acquireLock`. Fortunately, python/cpython#3385 simplified lock creation so that `with logging._lock:` is sufficient; that PR made it into Python 3.7, which is already pastescript's minimum Python requirement.
…eadlock on Python < 3.13 (#10104) * [k8s] Set urllib3 log level once per process instead of on every API call Previously, _api_logging_decorator re-decorated every method of the Kubernetes client on each API getter call, so every k8s API method invocation entered the set_logging_level context manager and called Logger.setLevel() twice (suppress + restore). setLevel() acquires the logging module's process-wide lock via Manager._clear_cache(); on Python < 3.13, a fork() while another thread holds that lock leaves it permanently held in the child, deadlocking the child's next logging call (fixed upstream in python/cpython#109462, which SkyPilot cannot rely on yet). Set the urllib3 logger level to ERROR once per process on first k8s API use instead. The old per-call set/restore was already racy under concurrent API calls (interleaved restores routinely left the level stuck at ERROR), so this preserves the effective behavior while shrinking the fork-deadlock window to a single one-time setLevel(). Also removes the per-call dir()/getattr sweep over the client wrapper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * [k8s] Drop redundant lock around one-time setLevel per review Set membership/add are GIL-atomic and a duplicate setLevel() from a racing first call is idempotent, so the double-checked lock is unnecessary — and it would itself be a lock a fork could leave permanently held in the child. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * [Core] Remove now-unused sky_logging.set_logging_level Its only caller was the kubernetes adaptor's per-call log suppression, removed earlier in this PR. The per-call set/restore pattern it enables is what made the pre-3.13 fork deadlock easy to hit, so drop it rather than leave it around to be reused. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * [Test] Add regression test for one-time urllib3 log suppression Verifies that a k8s API getter suppresses urllib3 warnings (via the effective level inherited by child loggers like urllib3.connectionpool) and that Logger.setLevel() is called exactly once per process across repeated and cross-API calls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * [k8s] Re-assert urllib3 log level after client construction Real-cluster testing (sky check against an unreachable context) showed urllib3 retry warnings still leaking: kubernetes.client.Configuration resets the urllib3 logger to WARNING in __init__ (its debug property setter), stomping the once-per-process level set. The old per-call set/restore masked this by re-setting the level at every method call, after construction. Re-assert the level after the decorated getter runs, guarded by a lock-free level read so setLevel() only fires when a client construction actually reset it — once per new client (per context / refresh), not per API call. Updated the regression test to simulate the Configuration reset, which the previous test missed by mocking _get_api_client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Updated logging library to acquire its module lock using a context manager so that it is safer and easier to use.