Skip to content
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

Handle KeyboardInterrupt during logging._acquireLock() #106238

Closed
arieleiz opened this issue Jun 29, 2023 · 1 comment
Closed

Handle KeyboardInterrupt during logging._acquireLock() #106238

arieleiz opened this issue Jun 29, 2023 · 1 comment
Labels
type-bug An unexpected behavior, bug, or error

Comments

@arieleiz
Copy link
Contributor

arieleiz commented Jun 29, 2023

We've come across a concurrency bug in logging/__init__.py which involves the handling of asynchronous exceptions, such as KeyboardInterrupt, during the execution of logging._acquireLock().

In the current implementation, when threading.RLock.acquire() is executed, there is a possibility for an asynchronous exception to occur during the transition back from native code, even if the lock acquisition is successful.

The typical use of _acquireLock() in the logging library is as follows:

def _loggingMethod(handler):
    """
    Add a handler to the internal cleanup list using a weak reference.
    """
    _acquireLock()
    try:
        # doSomething
    finally:
        _releaseLock()

In this pattern, if a KeyboardInterrupt is raised during the lock acquisition, the lock ends up getting abandoned.

When can this happen? One example is during forks. logging/__init__.py registers an at-fork hook, with

os.register_at_fork(before=_acquireLock,
                    after_in_child=_after_at_fork_child_reinit_locks,
                    after_in_parent=_releaseLock)

A scenario occurring in our production environment is during a slow fork operation (when the server is under heavy load and performing a multitude of forks). The lock could be held for up to a minute. If this is happening in a secondary thread, and a SIGINT signal is received in the main thread while is waiting to acquire the lock for logging, the lock will be abandoned. This will causes the process to hang during the next _acquireLock() call.

To address this issue, we provide a simple pull request to add a try-except block within _acquireLock(), e.g.:

def _acquireLock():
    if _lock:
        try:
            _lock.acquire()
        except BaseException:
            _lock.release()
            raise

This way, if an exception arises during the lock acquisition, the lock will be released, preventing the lock from being abandoned and the process from potentially hanging.

Linked PRs

@vstinner
Copy link
Member

See PR #109462 which changes how the logging acquire/release locks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants