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

gh-128384: Add locking to warnings.py. #128386

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Dec 31, 2024

The warnings module has some (relatively minor) thread safety issues. The catch_warnings contex manager is a major issue but that will be handled in a different PR. There are races between updating the filters list and incrementing the _filters_version number. Also, the warn_explicit() uses the global state in a non-thread-safe way. These issues are relatively easy to fix with some extra locking.

Changes:

  • expose the mutex used by the _warnings module, as _acquire_lock and _release_lock
  • implement a lock context manager and lock module state as needed
  • fix a bug in DeprecatedTests that resulted in mixing py_warnings and c_warnings modules.

The mutex used is non-reentrant and so some care is required to avoid deadlocks. I restructured the code in warn_explicit() to reduce functions called within the locked section. Perhaps a re-entrant lock should be used instead?

Expose the mutex from _warnings.c and hold it when mutating the filters
list.
@kumaraditya303
Copy link
Contributor

The mutex used is non-reentrant and so some care is required to avoid deadlocks. I restructured the code in warn_explicit() to reduce functions called within the locked section. Perhaps a re-entrant lock should be used instead?

I think the lock should be re-entrant, as locks are held when doing dict lookups which can call arbitrary python code.

Python/_warnings.c Outdated Show resolved Hide resolved
Lib/warnings.py Outdated Show resolved Hide resolved
Python/_warnings.c Outdated Show resolved Hide resolved
Python/_warnings.c Outdated Show resolved Hide resolved
Python/_warnings.c Outdated Show resolved Hide resolved
Python/_warnings.c Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants