-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-42815: Fix issue when thread doesn't copy context of parent thread #24074
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
Changes from all commits
24771bc
720a144
f57de91
51c8a44
ab9eff1
1fa0902
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
from time import monotonic as _time | ||
from _weakrefset import WeakSet | ||
from itertools import islice as _islice, count as _count | ||
from contextvars import copy_context as _copy_context | ||
try: | ||
from _collections import deque as _deque | ||
except ImportError: | ||
|
@@ -818,7 +819,7 @@ class Thread: | |
_initialized = False | ||
|
||
def __init__(self, group=None, target=None, name=None, | ||
args=(), kwargs=None, *, daemon=None): | ||
args=(), kwargs=None, *, daemon=None, context=None): | ||
"""This constructor should always be called with keyword arguments. Arguments are: | ||
|
||
*group* should be None; reserved for future extension when a ThreadGroup | ||
|
@@ -863,6 +864,10 @@ class is implemented. | |
else: | ||
self._daemonic = current_thread().daemon | ||
self._ident = None | ||
if context is not None: | ||
self._context = context | ||
else: | ||
self._context = _copy_context() | ||
if _HAVE_THREAD_NATIVE_ID: | ||
self._native_id = None | ||
self._tstate_lock = None | ||
|
@@ -943,11 +948,11 @@ def run(self): | |
""" | ||
try: | ||
if self._target is not None: | ||
self._target(*self._args, **self._kwargs) | ||
self._context.run(self._target, *self._args, **self._kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's one why to fix the issue. Another would be to modify the contextvars module to detect that it runs in a new thread. For exemple, this change doesn't cover the case of threads spawned by C libraries which then use the Python C API to run Python code. Or functions calling _thread.start_new_thread() directly. I don't know which option is the best. At least, this change is reasonable small. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, maybe I add bad description for this issue. I will try to make things more clear. Every thread has it's own context and they do not share same context. This PR tries fix problem when child thread use empty context instead of copy of a parent context. |
||
finally: | ||
uriyyo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Avoid a refcycle if the thread is running a function with | ||
# an argument that has a member that points to the thread. | ||
del self._target, self._args, self._kwargs | ||
del self._context, self._target, self._args, self._kwargs | ||
|
||
def _bootstrap(self): | ||
# Wrapper around the real bootstrap code that ignores | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Fix issue when new thread doesn't copy context of a parent thread. Patch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This description is not very helpful. I would prefer to rephrase it to explain that threads no longer share contextvars variables with their parent thread, but the context is now copied. IMO this change is important enough to be documented in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually every thread has it's own context and they do not share same context. This PR tries fix problem when child thread use empty context instead of copy of a parent context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rephrase the NEWS entry and add the requested versionchanged markup. |
||
provided by Yurii Karabas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is _copy_context() fast/efficient when contextvars is not used at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is information from PEP-567:
I believe it is fast/efficient to use
copy_context
.