✨ Add support for copying `contextvars` to thread workers #2160

tiangolo · 2021-10-27T20:42:28Z

✨ Add support for copying contextvars to thread workers.

This is my attempt at implementing what's described here: #648

The original post said:

run_sync_in_worker_thread should do copy_context in the trio thread and then ctx.run(...) in the worker thread

And it should unset the sniffio magic contextvar in the child

BlockingTrioPortal should do copy_context in the blocking thread, and then use the context in Trio

But Trio doesn't currently have a way to say "use this context for this task", so I guess we'd need to add that. A context= argument to spawn_system_task would work, and we might as well add it to start_soon while we're at it.

I guess these will also want to set up the sniffio magic contextvar. Maybe they should always copy the context that's passed in, and then mutate the copy?

I looked at the release notes and realized these names have been deprecated/replaced by other things:

run_sync_in_worker_thread -> to_thread.run_sync
BlockingTrioPortal -> from_thread.run_sync and from_thread.run

I'm also assuming that the "sniffio magic contextvar" refers to sniffio.current_async_library_cvar.

codecov · 2021-10-27T20:44:20Z

Codecov Report

Merging #2160 (02966f1) into master (53f6b74) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2160      +/-   ##
==========================================
+ Coverage   99.57%   99.62%   +0.04%     
==========================================
  Files         114      115       +1     
  Lines       14733    14861     +128     
  Branches     2335     2339       +4     
==========================================
+ Hits        14671    14805     +134     
+ Misses         41       38       -3     
+ Partials       21       18       -3

Impacted Files	Coverage Δ
trio/_core/_run.py	`100.00% <100.00%> (ø)`
trio/_threads.py	`100.00% <100.00%> (ø)`
trio/tests/test_contextvars.py	`100.00% <100.00%> (ø)`
trio/tests/test_threads.py	`100.00% <100.00%> (ø)`
trio/tests/test_ssl.py	`99.85% <0.00%> (+0.56%)`	⬆️
trio/_highlevel_ssl_helpers.py	`100.00% <0.00%> (+11.76%)`	⬆️

tiangolo · 2021-10-27T22:04:09Z

I'm not sure what to do about the Fedora CI broken build 🤔

I imagine this also requires an update in release notes, etc. But I would like to get some feedback first, check if anything seems wrong to anyone here, etc.

trio/_threads.py

richardsheridan

Aside from my comments (one of which leaked out of this review oops!) I think the question of adding context kwargs to the API should be re-confirmed by a few others.

trio/_threads.py

tiangolo · 2021-10-28T22:16:01Z

Thanks for the thorough review @richardsheridan! ☕

Yep, I would have probably not included explicit context kwargs in the API, but I tried to stick to what @njsmith wanted/wrote in the original issue: #648

I'll wait for more feedback. 🤓

pquentin · 2021-10-29T06:38:27Z

I merged master into your branch, this fixed the Fedora build!

pquentin · 2021-10-29T10:08:22Z

trio/_threads.py

+        func = cb
+        func_args = (q, fn, args)


Here's a more visual way to see the uncovered lines:

Thanks! I realized I'm already modifying the two calls that use this function, it's never called without a context, so I just made the context argument required, and that seems to have solved the coverage.

…rrent_async_library_cvar

…extvars

…andle coverage

tiangolo · 2021-10-29T17:51:35Z

Thanks for the reviews and comments @richardsheridan, @smurfix, and @pquentin! 🍰

I implemented the changes, tests, and updated the coverage.

I also just added the newsfragment and a bit of new simple documentation including a new simple example. Let me know what you think!

I think this would cover all the things necessary for the PR, am I missing something else?

Co-authored-by: richardsheridan <[email protected]>

tiangolo · 2021-11-15T16:12:03Z

Awesome, thanks for the review @richardsheridan! I agree, I would love to have @njsmith's point of view on this!

richardsheridan · 2022-01-27T19:55:33Z

If it were just me, I would merge the PR for the next release but without the kwarg change to Nursery.start_soon because it's redundant to context.run(nursery.start_soon) and a function signature change in the lowlevel package only is much more tolerable than in the core API. Also for what it's worth, it would keep things in sync with @agronholm choices in AnyIO.

newsfragments/2160.feature.rst

agronholm · 2022-01-27T20:35:28Z

If it were just me, I would merge the PR for the next release but without the kwarg change to Nursery.start_soon because it's redundant to context.run(nursery.start_soon) and a function signature change in the lowlevel package only is much more tolerable than in the core API. Also for what it's worth, it would keep things in sync with @agronholm choices in AnyIO.

I fully agree. @tiangolo would it be possible to modify this PR or create another one without the API changes?

as requested in code review

…eter

…newsfragment

newsfragments/2160.feature.rst

tiangolo · 2022-01-30T15:56:09Z

Awesome, thanks for the feedback @richardsheridan and @agronholm! 🙇‍♂️ 🚀

Done, I updated it accordingly. Let me know if you see anything else or if I missed something.

richardsheridan · 2022-01-30T15:57:59Z

The failing tests will likely be fixed by merging master.

agronholm · 2022-01-30T16:10:02Z

trio/_core/_run.py

@@ -1540,7 +1543,7 @@ def task_exited(self, task, outcome):
    ################

    @_public
-    def spawn_system_task(self, async_fn, *args, name=None):
+    def spawn_system_task(self, async_fn, *args, name=None, context=None):


I don't think this is supposed to be here anymore? System tasks should always inherit the system context, right?

I actually haven't been able to make tests pass without it, I've been trying several different approaches, copying the context in different places, but in some I get errors that show it's not copying the right context, or thread timeout errors, or it breaks internals. 😱 I suspect it could be just that I lack the knowledge of the internals to do it in a different and better way.

Do you see a way that could be solved to remove it?

On the other hand, I understand @richardsheridan thinks this could be useful... for me it's the same, but I just don't see how to make it work without this. 🤔

richardsheridan · 2022-01-30T16:12:22Z

I think that's a good feature, otherwise there is no way to inject any other context to a system task...

…

On Sun, Jan 30, 2022, 11:10 AM Alex Grönholm ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In trio/_core/_run.py <#2160 (comment)>: > @@ -1540,7 +1543,7 @@ def task_exited(self, task, outcome): ################ @_public - def spawn_system_task(self, async_fn, *args, name=None): + def spawn_system_task(self, async_fn, *args, name=None, context=None): I don't think this is supposed to be here anymore? System tasks should always inherit the system context, right? — Reply to this email directly, view it on GitHub <#2160 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5BHRSUGNPVHRB3ACB37SLUYVPGLANCNFSM5G3IHZUQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

tiangolo · 2022-01-30T16:21:44Z

I just noticed this warning in _core/_generated_run.py: https://github.com/python-trio/trio/blob/master/trio/_core/_generated_run.py#L1-L3

But I can't see what generates it, the only reference to generated_run I get searching in my editor is the import in _run.py, do you know what's that note about? Or what should I modify instead?

richardsheridan · 2022-01-30T16:50:47Z

Right you should run gen_exports.py after making changes to public apis in _run.py and some others. It seemed like you were already doing that!

agronholm · 2022-01-30T17:20:14Z

I think that's a good feature, otherwise there is no way to inject any other context to a system task...
…
On Sun, Jan 30, 2022, 11:10 AM Alex Grönholm @.> wrote: @.* commented on this pull request. ------------------------------ In trio/_core/_run.py <#2160 (comment)>: > @@ -1540,7 +1543,7 @@ def task_exited(self, task, outcome): ################ @_public - def spawn_system_task(self, async_fn, args, name=None): + def spawn_system_task(self, async_fn, args, name=None, context=None): I don't think this is supposed to be here anymore? System tasks should always inherit the system context, right? — Reply to this email directly, view it on GitHub <#2160 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5BHRSUGNPVHRB3ACB37SLUYVPGLANCNFSM5G3IHZUQ . You are receiving this because you were mentioned.Message ID: @.*>

That API addition is not mentioned in the news fragment.

tiangolo · 2022-01-30T18:48:25Z

That API addition is not mentioned in the news fragment.

@agronholm get it, just updated it. I suspect only about trio.lowlevel.spawn_system_task, right? That's what I added to the newsfragment.

newsfragments/2160.feature.rst

tiangolo · 2022-02-14T10:30:35Z

Thanks @pquentin! 🚀

oremanj · 2023-02-21T20:08:58Z

This PR introduced a regression: calling spawn_system_task now changes the current_async_library_cvar in the calling task too, even if that's in a different async library such as asyncio. This violates the rule that it is generally fine to do synchronous cross-calls between two event loops in the same thread without additional ceremony.

richardsheridan · 2023-06-29T18:30:22Z

This PR introduced a regression: calling spawn_system_task now changes the current_async_library_cvar in the calling task too, even if that's in a different async library such as asyncio. This violates the rule that it is generally fine to do synchronous cross-calls between two event loops in the same thread without additional ceremony.

For my future self's reference: This was regression resulted in #2183 and was fixed in #2462.

tiangolo added 4 commits October 27, 2021 21:41

✨ Add support for passing contextvars to thread workers

bb24a6b

✅ Add tests for passing contextvars to threads

9a149d6

✨ Add support for custom contextvars context for start_soon

ac32907

✅ Add tests for contextvars with start_soon

d678e7b

tiangolo added 2 commits October 27, 2021 23:34

🐛 Fix unnecessary wrapper

5c2b5d0

✅ Fix tests with unordered results

2bfde96

tiangolo mentioned this pull request Oct 27, 2021

We should do a better job of preserving contextvars across mode switches #648

Open

richardsheridan reviewed Oct 28, 2021

View reviewed changes

trio/_threads.py Show resolved Hide resolved

richardsheridan requested changes Oct 28, 2021

View reviewed changes

trio/_threads.py Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into contextvars-threads

1b8210d

pquentin reviewed Oct 29, 2021

View reviewed changes

tiangolo added 11 commits October 29, 2021 12:28

♻️ Move contextvars context out of partial and into a closure

6e9935b

🔥 Remove unused import functools

7ddd95c

✅ Add test for sniffio current_async_library_cvar unsetting

0a9fc41

⏪ Revert moving context copy out of partial as that breaks sniffio cu…

ac46161

…rrent_async_library_cvar

✅ Add tests for sniffio contextvar when using worker threads and cont…

4891d28

…extvars

🐛 Fix setting the sniffio contextvar in from_thread.run

6541f15

♻️ Refactor utility function to require context (always passed) and h…

ac36a90

…andle coverage

🎨 Fix formatting

fe83fd6

📝 Add newsfragment

3250e53

🍱 Add example of contextvars with threads

a268435

📝 Add docs for contextvars with threads

83e1b2e

tiangolo added 2 commits October 29, 2021 20:32

📝 Fix docs references to internal functions

4b3dd53

📝 Fix internal function references in newsfragment

33202e9

pquentin requested a review from richardsheridan November 5, 2021 06:13

tiangolo and others added 2 commits November 15, 2021 11:08

✏ Update docs/source/reference-core.rst

3cc0029

Co-authored-by: richardsheridan <[email protected]>

✏ Update docs/source/reference-core.rst

81320fc

Co-authored-by: richardsheridan <[email protected]>

tiangolo mentioned this pull request Nov 15, 2021

✨ Add support for copying contextvars to thread workers agronholm/anyio#389

Closed

agronholm mentioned this pull request Nov 17, 2021

Added propagation of contextvars to/from threads agronholm/anyio#390

Merged

agronholm reviewed Jan 27, 2022

View reviewed changes

newsfragments/2160.feature.rst Outdated Show resolved Hide resolved

Update newsfragments/2160.feature.rst

4309dcd

tiangolo added 3 commits January 30, 2022 16:38

🔥 Remove context parameter from nursery.start_soon

1e9af84

as requested in code review

🔥 Remove mention in docs to removed start_soon(context=context) param…

53ae1a1

…eter

🔥 Remove mention to discarded nursery.start_soon(context=context) in …

0a388bb

…newsfragment

agronholm reviewed Jan 30, 2022

View reviewed changes

newsfragments/2160.feature.rst Outdated Show resolved Hide resolved

agronholm reviewed Jan 30, 2022

View reviewed changes

📝 Update newsfragment

e609329

🔀 Merge master

2eed34c

richardsheridan reviewed Jan 30, 2022

View reviewed changes

newsfragments/2160.feature.rst Outdated Show resolved Hide resolved

fix rtd with double backticks

02966f1

pquentin merged commit d88acd2 into python-trio:master Jan 31, 2022

tiangolo deleted the contextvars-threads branch February 14, 2022 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add support for copying `contextvars` to thread workers #2160

✨ Add support for copying `contextvars` to thread workers #2160

tiangolo commented Oct 27, 2021

codecov bot commented Oct 27, 2021 •

edited

Loading

tiangolo commented Oct 27, 2021

richardsheridan left a comment

tiangolo commented Oct 28, 2021

pquentin commented Oct 29, 2021 •

edited

Loading

pquentin Oct 29, 2021

tiangolo Oct 29, 2021

tiangolo commented Oct 29, 2021

tiangolo commented Nov 15, 2021

richardsheridan commented Jan 27, 2022

agronholm commented Jan 27, 2022

tiangolo commented Jan 30, 2022

richardsheridan commented Jan 30, 2022 •

edited

Loading

agronholm Jan 30, 2022

tiangolo Jan 30, 2022

richardsheridan commented Jan 30, 2022 via email

tiangolo commented Jan 30, 2022

richardsheridan commented Jan 30, 2022

agronholm commented Jan 30, 2022

tiangolo commented Jan 30, 2022

tiangolo commented Feb 14, 2022

oremanj commented Feb 21, 2023

richardsheridan commented Jun 29, 2023

✨ Add support for copying contextvars to thread workers #2160

✨ Add support for copying contextvars to thread workers #2160

Conversation

tiangolo commented Oct 27, 2021

codecov bot commented Oct 27, 2021 • edited Loading

Codecov Report

tiangolo commented Oct 27, 2021

richardsheridan left a comment

Choose a reason for hiding this comment

tiangolo commented Oct 28, 2021

pquentin commented Oct 29, 2021 • edited Loading

pquentin Oct 29, 2021

Choose a reason for hiding this comment

tiangolo Oct 29, 2021

Choose a reason for hiding this comment

tiangolo commented Oct 29, 2021

tiangolo commented Nov 15, 2021

richardsheridan commented Jan 27, 2022

agronholm commented Jan 27, 2022

tiangolo commented Jan 30, 2022

richardsheridan commented Jan 30, 2022 • edited Loading

agronholm Jan 30, 2022

Choose a reason for hiding this comment

tiangolo Jan 30, 2022

Choose a reason for hiding this comment

richardsheridan commented Jan 30, 2022 via email

tiangolo commented Jan 30, 2022

richardsheridan commented Jan 30, 2022

agronholm commented Jan 30, 2022

tiangolo commented Jan 30, 2022

tiangolo commented Feb 14, 2022

oremanj commented Feb 21, 2023

richardsheridan commented Jun 29, 2023

✨ Add support for copying `contextvars` to thread workers #2160

✨ Add support for copying `contextvars` to thread workers #2160

codecov bot commented Oct 27, 2021 •

edited

Loading

pquentin commented Oct 29, 2021 •

edited

Loading

richardsheridan commented Jan 30, 2022 •

edited

Loading