bpo-42815: Fix issue when thread doesn't copy context of parent thread #24074

uriyyo · 2021-01-03T11:46:38Z

https://bugs.python.org/issue42815

github-actions · 2021-02-10T00:23:38Z

This PR is stale because it has been open for 30 days with no activity.

# Conflicts: # Lib/test/test_threading.py # Lib/threading.py

jcea · 2021-07-31T02:59:51Z

Could you possibly explain why do you need to amend decimal tests?. I don't understand it.

jcea · 2021-07-31T03:34:42Z

Oh, I understand now. You want to preserve the behaviour of a thread messing with decimal context to affect other threads (including main thread).

I find this patch valuable, but the decimal context "interference" in current python could be something production code could depend on. In that situation, this patch is "risky" and we are possibly too late for 3.10, since 3.10rc1 is planned in a couple of days.

Maybe Python 3.11?

We also need a documentation update talking about the new "context" optional parameter.

I requested a review in python-dev.

jcea

Test should check that old context is altered when passing the old context as a parameter, and not altered when using an implicit new context.
The same when providing an explicit new context.

bedevere-bot · 2021-07-31T03:43:22Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner

cc @1st1, author of the contextvars module.

vstinner · 2021-08-04T14:43:22Z

Lib/threading.py

@@ -925,7 +930,7 @@ 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)


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.

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.

vstinner · 2021-08-04T14:43:57Z

Lib/threading.py

+        if context is not None:
+            self._context = context
+        else:
+            self._context = _copy_context()


Is _copy_context() fast/efficient when contextvars is not used at all?

This is information from PEP-567:

The Context mapping is implemented using an immutable dictionary. This allows for a O(1) implementation of the copy_context() function.

I believe it is fast/efficient to use copy_context.

vstinner · 2021-08-04T14:45:11Z

Misc/NEWS.d/next/Library/2021-01-03-13-44-44.bpo-42815.IJqHPr.rst

@@ -0,0 +1,2 @@
+Fix issue when new thread doesn't copy context of a parent thread. Patch


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 .. versionchanged:: 3.11 markup in https://docs.python.org/dev/library/threading.html#threading.Thread documentation.

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.

Please rephrase the NEWS entry and add the requested versionchanged markup.

uriyyo · 2021-08-10T12:00:47Z

@jcea 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.

For instead, I can that this problem fixed at asyncio.to_thread:

cpython/Lib/asyncio/threads.py

Lines 22 to 25 in c7ea1e3

    
           loop = events.get_running_loop() 
        
           ctx = contextvars.copy_context() 
        
           func_call = functools.partial(ctx.run, func, *args, **kwargs) 
        
           return await loop.run_in_executor(None, func_call)

jcea · 2021-08-10T13:21:49Z

@uriyyo , your comment doesn't make sense if a thread modifying its context affects what other threads see. That mean "shared context" in my book :-).

The comment by @vstinner about threads launched by C extensions is interesting. I was thinking about detecting this case when getting the GIL and copy the context at that time. The problem would be to decide which context to copy, in the general case.

Maybe perfection is enemy of good and this PR would be good enough for now.

Lib/threading.py

# Conflicts: # Lib/test/test_threading.py

vstinner · 2021-08-11T09:21:51Z

Currently, the _contextvars module is built as a dynamic library, it is not available when setup.py build is run by make to build Python, and so import logging fails when trying to get it indirectly.

I suggest to modify Modules/Setup to uncomment #_contextvars (...), so the _contextvars module is built as a built-in module, and so is always available.

uriyyo · 2022-12-13T14:24:51Z

Closing this PR as far as a related issue has been closed: #86981 (comment)

Fix issue when thread doesn't copy context of parent thread

24771bc

the-knights-who-say-ni added the CLA signed label Jan 3, 2021

bedevere-bot added the awaiting review label Jan 3, 2021

Fix deciamal threading tests

720a144

uriyyo requested a review from rhettinger as a code owner January 3, 2021 14:28

rhettinger removed their request for review January 10, 2021 20:02

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 10, 2021

Merge remote-tracking branch 'origin/master' into fix-issue-42815

f57de91

# Conflicts: # Lib/test/test_threading.py # Lib/threading.py

jcea requested changes Jul 31, 2021

View reviewed changes

bedevere-bot removed the awaiting review label Jul 31, 2021

bedevere-bot added the awaiting changes label Jul 31, 2021

pablogsal requested a review from 1st1 July 31, 2021 03:51

vstinner reviewed Aug 4, 2021

View reviewed changes

vstinner reviewed Aug 11, 2021

View reviewed changes

Lib/threading.py Show resolved Hide resolved

uriyyo added 2 commits August 11, 2021 12:00

Merge remote-tracking branch 'origin/master' into fix-issue-42815

51c8a44

# Conflicts: # Lib/test/test_threading.py

Delete context to avoid refcycle

ab9eff1

Uncomment _contextvars at Modules/Setup

1fa0902

uriyyo mentioned this pull request Apr 10, 2022

new thread doesn't copy context of the parent thread #86981

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 2, 2022

uriyyo closed this Dec 13, 2022

		@@ -0,0 +1,2 @@
		Fix issue when new thread doesn't copy context of a parent thread. Patch

Uh oh!

bpo-42815: Fix issue when thread doesn't copy context of parent thread #24074

bpo-42815: Fix issue when thread doesn't copy context of parent thread #24074

Uh oh!

Conversation

uriyyo commented Jan 3, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 10, 2021

Uh oh!

jcea commented Jul 31, 2021

Uh oh!

jcea commented Jul 31, 2021

Uh oh!

jcea left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 31, 2021

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

uriyyo Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

uriyyo Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

uriyyo Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

uriyyo commented Aug 10, 2021

Uh oh!

jcea commented Aug 10, 2021

Uh oh!

Uh oh!

vstinner commented Aug 11, 2021

Uh oh!

uriyyo commented Dec 13, 2022

Uh oh!

Uh oh!

uriyyo commented Jan 3, 2021 •

edited by bedevere-bot

Loading