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

[njsmith]

When run_sync_in_worker_thread or BlockingTrioPortal switch between the trio thread and a worker thread, they should preserve the contextvars.Context.

Using literally the same Context turns out to be quite tricky though, because Context.run enforces that only one thread can be inside a given Context object at any given time. For run_sync_in_worker_thread this is doable in theory but tricky (you have to make sure that the worker thread waits for the parent task to have suspended itself before it calls Context.run), and for BlockingTrioPortal is basically not possible. Also, if PEP 568 is merged, then we'll actually have a context stack that we would want to be preserving, and that makes things complicated too.

Instead, we should probably say that these mode switching functions copy the context (which is super cheap). This means the call would see all the same contextvars that the parent does, except that mutations in the child would not be propagated back to the parent. Which is maybe a tiny wart conceptually but seems fine in practice.

So:

• 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?

pytest-trio would also use a start_soon(..., context=...) kwarg to share contexts between fixtures: https://github.com/python-trio/pytest-trio/issues/59 – but it wants start_soon to not make a copy of the context, so maybe that's better and we should either (a) document that the Context will be mutated to set the sniffio state, or (b) document that people who use context= need to set the sniffio state correctly themselves.

I guess that if PEP 568 happens and we start storing cancel state inside the context (to make it safe to use cancel scopes inside generators, see #264), then we'll need to fix up the cancel scope state whenever we get a context= passed in from outside. So probably better to be consistent and either go ahead and mutate the sniffio state too, or else make the copy and mutate it (and pytest-trio would just have to find a way to live with copies, which I think it can do). ...and actually I guess if we start storing cancel state inside the context then pytest-trio will have to stop using its current hack.

Also, if we had PEP 568, then I guess we wouldn't need to mutate the context to handle sniffio state; instead we could stash the sniffio state in some lower-level context on the context stack, instead of inside the individual task contexts.

Hmm. I'm pretty indecisive about copying the context versus not. Not copying is more direct (the thing you pass as context= is the context), and versatile (if people want copies they can do that themselves). These are both good for a weird edge-case thing like this. But OTOH copying is a kind of safer, less error-prone kind of thing to do, which is also good for a weird edge-case thing like this.

[smurfix]

Personally I'm all for copying the context. It's a different environment, after all, and I won't expect anything (besides the return value / exception) to implicitly pass back to the caller.

[tiangolo]

I added a possible implementation in this PR https://github.com/python-trio/trio/pull/2160

---

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 would also imagine that the "sniffio magic contextvar" refers to sniffio.current_async_library_cvar that seems to tell the current async backend.

I'm also adding this comment here in case that PR doesn't work for some reason. I would think the information about the new names might be useful still. 🤓