Skip to content

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.

@nascheme nascheme marked this pull request as ready for review January 6, 2025 18:42
…0xzwH.rst Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@kumaraditya303
Copy link
Contributor

This looks good now from a brief look.

@nascheme nascheme merged commit 1c13c56 into python:main Jan 14, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants