Add API for changing the context of a running task. #2086

Fuyukai · 2021-08-01T01:57:50Z

This is implemented as an asynchronous context manager (allowing arbitrary nesting).

codecov · 2021-08-01T01:59:30Z

Codecov Report

Merging #2086 (24b3814) into master (f8cb817) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2086   +/-   ##
=======================================
  Coverage   99.55%   99.55%           
=======================================
  Files         114      116    +2     
  Lines       14751    14785   +34     
  Branches     2341     2344    +3     
=======================================
+ Hits        14685    14719   +34     
  Misses         44       44           
  Partials       22       22

Impacted Files	Coverage Δ
trio/lowlevel.py	`100.00% <ø> (ø)`
trio/_core/__init__.py	`100.00% <100.00%> (ø)`
trio/_core/_context.py	`100.00% <100.00%> (ø)`
trio/_core/tests/test_context.py	`100.00% <100.00%> (ø)`

smurfix · 2021-08-01T07:32:26Z

Hmm, I'd call this function changed_context so that the with statement reads like a reasonable English phrase.

Fuyukai · 2021-08-01T07:53:01Z

That's inconsistent with every other context manager in the library.

Fuyukai · 2021-08-04T11:45:10Z

If there's no objections to this in the next four days I'm going to merge this unilaterally.

goodboy

Looks fine to me; any lower level testing is just testing contextvars itself 😂

I wonder would an example of usage in a real trio example be of note?
I can't think of where I'd use this offhand so it'd be handy to have a notion up front where you'd expect to have it used.

oremanj

Thanks for the change! I have some mixed feelings about the "I'm going to commit this regardless of review once it's been up for a week" -- I think based on typical review habits in this project that a week of silence doesn't actually constitute a mandate to go ahead. I can't deny it prompted me to review ASAP, but it felt a little coercive? I'd be more open to this sort of thing if we agree on a standard.

oremanj · 2021-08-04T13:53:40Z

newsfragments/2086.feature.rst

@@ -0,0 +1,2 @@
+Add an API to allow changing the current Context a task is running
+in.


Link to the new API here? And cross-reference contextvars.Context

oremanj · 2021-08-04T13:53:55Z

trio/__init__.py

@@ -32,6 +32,7 @@
    BrokenResourceError,
    EndOfChannel,
    Nursery,
+    change_context,


I think the public export of this belongs in lowlevel

I was very unsure if this should be in public or lowlevel, I basically did a mental coin flip. I'll change it back.

oremanj · 2021-08-04T13:57:35Z

trio/_core/_context.py

+
+@asynccontextmanager
+async def change_context(context):
+    """Asynchronous context manager to change the :class:`contextvars.Context`


suggest "Returns an async context manager that can be used to change the ... to match the style of the docs on other functions that do that

oremanj · 2021-08-04T13:59:16Z

trio/_core/_context.py

+    task.context = context
+
+    try:
+        await _run.checkpoint()


I think these should both be cancel_shielded_checkpoint in case the user wants to change the context without taking a cancellation. This is a pretty low-level feature so I'm not worried about deviation from the "entry and exit to ACM are checkpoints" rule. (This is probably more justifiable if you take the suggestion to put the function in lowlevel.)

Regardless, you should document what happens on entry and on exit wrt cancellation and scheduling.

oremanj · 2021-08-04T14:00:23Z

trio/_core/_context.py

+        yield
+    finally:
+        task.context = saved_context
+        # I assume this is right?


Entry and exit are symmetric IMO; you need a schedule point to pick up the new context, but there aren't any correctness implications I can see about whether it should also be a cancel point or not.

The second one is cancel-shielded because it would produce a second cancellation event if the stuff inside was cancelled. The first one isn't because it seems better to not have to do two round trips through the scheduler if the task is already cancelled.

That's fair -- I guess anyone who wants to change the context without taking a cancellation can also use a shielded scope. I think most permutations here are defensible as long as they're documented.

oremanj · 2021-08-04T14:00:41Z

trio/_core/_context.py

+
+
+@asynccontextmanager
+async def change_context(context):


perhaps change_current_context or set_current_context to make the functionality clearer from the name?

oremanj · 2021-08-04T14:03:16Z

docs/source/reference-core.rst

@@ -1020,6 +1020,12 @@ Example output (yours may differ slightly):
   request 0: Helper task b finished
   request 0: Request received finished

+You can change the current :class:`contextvars.Context` a task is running


This feels out of place when the prior discussion didn't even mention the concept of a Context (only discussed context variables). That's part of why I think this might do better in lowlevel. We have #1543 that can keep it company (once cleaned up and landed) in some sort of "Low-level context tools" section.

Okay, I'll move it to the low-level docs as its own section.

Fuyukai · 2021-08-04T15:14:35Z

I have some mixed feelings about the "I'm going to commit this regardless of review once it's been up for a week" -- I think based on typical review habits in this project that a week of silence doesn't actually constitute a mandate to go ahead.

This feature is blocking the project I'm working on. Given that it's a ten line feature change, relatively simple, and that the review was primarily about semantics I think that it's fair to expect this could be pushed through quickly.

oremanj · 2021-08-04T15:20:30Z

This feature is blocking the project I'm working on. Given that it's a ten line feature change, relatively simple, and that the review was primarily about semantics I think that it's fair to expect this could be pushed through quickly.

You could inline change_context() into your project in the meantime -- it doesn't actually use any private APIs. I'm not trying to pass judgment on whether pushing on this change's timeline was abstractly fair or not; mostly wanted to flag that I was personally somewhat taken aback by the approach.

pquentin · 2021-08-13T13:08:40Z

@Fuyukai any progress on this?

Fuyukai · 2021-12-23T04:24:36Z

Hi, I picked this back up again after ~five months of being a writer :)

I addressed all the comments raised by @oremanj. I also wish to apologise for my rash behaviour and threatening a unilateral merge.

goodboy approved these changes Aug 4, 2021

View reviewed changes

oremanj requested changes Aug 4, 2021

View reviewed changes

Fuyukai added 5 commits December 23, 2021 03:53

Add API for changing the context of a running task.

9ee445e

Export change_context.

506d7c8

Newsfragment + doc.

885b888

Blackify.

4f9736d

Address review comments.

24b3814

Fuyukai closed this by deleting the head repository Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API for changing the context of a running task. #2086

Add API for changing the context of a running task. #2086

Fuyukai commented Aug 1, 2021

codecov bot commented Aug 1, 2021 •

edited

Loading

smurfix commented Aug 1, 2021

Fuyukai commented Aug 1, 2021

Fuyukai commented Aug 4, 2021

goodboy left a comment •

edited

Loading

oremanj left a comment

oremanj Aug 4, 2021

oremanj Aug 4, 2021

Fuyukai Aug 4, 2021

oremanj Aug 4, 2021

oremanj Aug 4, 2021

oremanj Aug 4, 2021

Fuyukai Aug 4, 2021

oremanj Aug 4, 2021

oremanj Aug 4, 2021

oremanj Aug 4, 2021

Fuyukai Aug 4, 2021

Fuyukai commented Aug 4, 2021

oremanj commented Aug 4, 2021

pquentin commented Aug 13, 2021

Fuyukai commented Dec 23, 2021

		@@ -0,0 +1,2 @@
		Add an API to allow changing the current Context a task is running
		in.

Add API for changing the context of a running task. #2086

Add API for changing the context of a running task. #2086

Conversation

Fuyukai commented Aug 1, 2021

codecov bot commented Aug 1, 2021 • edited Loading

Codecov Report

smurfix commented Aug 1, 2021

Fuyukai commented Aug 1, 2021

Fuyukai commented Aug 4, 2021

goodboy left a comment • edited Loading

Choose a reason for hiding this comment

oremanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fuyukai commented Aug 4, 2021

oremanj commented Aug 4, 2021

pquentin commented Aug 13, 2021

Fuyukai commented Dec 23, 2021

codecov bot commented Aug 1, 2021 •

edited

Loading

goodboy left a comment •

edited

Loading