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: Use a context variable for warnings.catch_warnings #130010

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Feb 11, 2025

Make warnings.catch_warnings() use a context variable for holding the warning filtering state if the sys.flags.context_aware_warnings flag is set to true. This makes using the context manager thread-safe in multi-threaded programs and safe for asyncio coroutines and tasks. The flag is true by default in free-threaded builds and is otherwise false. The value of the flag can be overridden by the the -X thread_safe_warnings command-line option or by the PYTHON_CONTEXT_AWARE_WARNINGS environment variable.

It is expected that one day the flag might default to true for all Python builds, not just the free-threaded one. However, I think it makes sense to not commit to a schedule to do that until we have a better idea of what code is affected by this change. Feedback from people using the free-threaded build should provide guidance.

This PR is on top of gh-128209.

A previous version of this PR used a single flag to control both behavior of Thread inheriting the context and also the catch_warnings context manager. However, based on some feedback, I've decided that two flags makes things more clear. Likely programs would like to set both flags to true to get the most intuitive behavior.

When the context_aware_warnings flag is true and a catch_warnings() context is active, added filters go into a list of filters stored in the contextvar, not the warnings.filters list. The filters in warnings.filters are still applied but they apply after the contextvar filters.

That difference with how warnings.filters works is probably the most likely thing to cause broken user code. In the unit tests, I had to change a number of references to warnings.filters to warnings._get_filters(). That function returns the list of filters from the current context or the global filters if there is no context active. Perhaps _get_filters() should become a public function? I think it would be better if examining and manipulating that list was discouraged and so that's why I made the function non-public, at least for now.

I created _py_warnings.py to contain the Python implementation for the warnings module. This matches other modules like decimal that have Python and C versions of the same module. In the case of warnings things are a bit more complicated since you can assign to module globals (the filters list or the showwarning() function are common to override). This is a cleaner organization, IMHO. Now warnings.py just imports the parts it needs and the unit tests are a bit simpler.

I cleaned up the unit tests for warnings a bit. Passing module to catch_warnings() actually serves no purpose. Since sys.modules['warnings'] is already swapped to the module under test, the code does the right thing even when that parameter is not given. I also replaced calls to original_warnings.catch_warnings() with self.module.catch_warnings(). Both those calls do the same thing but the second spelling seems less confusing to me.

pyperformance comparison


📚 Documentation preview 📚: https://cpython-previews--130010.org.readthedocs.build/

Since the unittest/runner.py establishes a new warnings context with
`catch_warnings`, these tests must use `warnings._get_filters()` to
retrieve the current list of filters, not the `warnings.filters` global.
@nascheme nascheme marked this pull request as ready for review February 13, 2025 21:03
@nascheme
Copy link
Member Author

I'm also curious if this makes any impact on a pyperformance run. thread startup gains a bit of new code with the context object creation and use. warnings might have a little more, but hopefully not critical path.

I added a link to a pyperformance comparison. Small enough to be within the noise, it seems, as I would suspect.

Using the critical section seems the easiest fix for this.  Add a
unit test that fails TSAN check if the fix is not applied.
@nascheme nascheme requested a review from gpshead March 11, 2025 23:20
@nascheme
Copy link
Member Author

nascheme commented Mar 11, 2025

Hello @gpshead please take another look if you can. I believe your feedback has been addressed. For notifying people of this change, the plan is to do that in the following places: first the what's new document can have a note for the 3.14 release; second, there are notes in the free-threaded howto (part of the help) and in the "warnings" and "threading" library docs.

@kumaraditya303
Copy link
Contributor

I think we should add some tests which use both asyncio and warnings to check for correct contexts in async environments too.

Comment on lines +167 to +173
In the free-threaded build, the flag :data:`~sys.flags.thread_inherit_context`
is set to true by default. In the default GIL-enabled build, the flag
defaults to false. This will cause threads created with
:class:`threading.Thread` to start with a copy of the
:class:`~contextvars.Context()` of the caller of
:meth:`~threading.Thread.start`. If the flag is false, threads start with an
empty :class:`~contextvars.Context()`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In the free-threaded build, the flag :data:`~sys.flags.thread_inherit_context`
is set to true by default. In the default GIL-enabled build, the flag
defaults to false. This will cause threads created with
:class:`threading.Thread` to start with a copy of the
:class:`~contextvars.Context()` of the caller of
:meth:`~threading.Thread.start`. If the flag is false, threads start with an
empty :class:`~contextvars.Context()`.
In the free-threaded build, the flag :data:`~sys.flags.thread_inherit_context`
is set to true by default which causes threads created with
:class:`threading.Thread` to start with a copy of the
:class:`~contextvars.Context()` of the caller of
:meth:`~threading.Thread.start`. In the default GIL-enabled build, the flag
defaults to false so threads start with an
empty :class:`~contextvars.Context()`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants