Skip to content
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

make TreeVar work in sync code #31

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

agnesnatasya
Copy link

This change makes TreeVar works in sync code, and it behaves similarly to a normal contextvar in sync context

Fixes #30

assert tv2.get() == 30
assert tv2.get() == 10

def mix_async_and_sync():
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my interpretation of its expected behavior when we mix sync and async context, let me know if any of it is not in line with what you have in mind!

Copy link
Owner

@oremanj oremanj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the effort, but doing this correctly is going to be substantially more complicated than what you've posted so far.

Your approach is implicitly assuming that there is only one synchronous context in the whole program. That's not true though. You get one per thread to start with. You can create more -- contextvars.Context has a public constructor.

If we're doing something reasonable when there is no Trio current task, we need to distinguish two levels of "async context":

  • If trio.current_time() fails, there's no active Trio run at all. This is the "synchronous" case. We should set the TreeVarState in the current context using _cvar.set().
  • If trio.current_task() fails but trio.current_time() doesn't, there is an active run but no active task. This state is observable primarily in instruments, end-of-run async generator finalizers, and some abort_fn callbacks. I think we should treat TreeVar modifications in this state as if they occurred in the trio.lowlevel.current_root_task(). They will not propagate to any other task, because the nurseries underneath the root task have already been opened by the time any user-provided code runs.

When you access a TreeVar inside a Trio run whose value was set outside, you should get the value that was set in the context where trio.run() was called. That's available using trio.lowlevel.current_root_task().context. (This is not actually the enclosing context, it's a copy, but Trio doesn't make any changes to the copy, so it's a good reflection of what things looked like when trio.run() was called.)

We should also consider inheritance across threads. If you start a new thread using trio.to_thread.run_sync, it gets a copy of your contextvars context. So if you think you're in synchronous context, but your fetch returns a _TreeVarState that comes from a Trio task, your inherited value should come from the task that started the thread (which is not necessarily the same as the task that your TreeVarState comes from). You can determine the TreeVar value in that task using trio.from_thread.run_sync(self.get) and treat that as your inherited value.


def __init__(self, name: str, *, default: T = MISSING):
self._cvar = contextvars.ContextVar(name)
self._default = default
# Dummy trio task that that simulates the 'current task'
# whenever we're running in a synchronous context
self._sync_context_task = trio.lowlevel.Task._create(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really OK with this use of private APIs to create a trio task that trio doesn't know about. You'll need to figure out a different approach. I also see some red flags here like:

  • why does every individual TreeVar need its own separate sync context? In reality isn't there only usually one context per thread in sync code?
  • why are we capturing copy_context() when the TreeVar is created? That might be different than the context of interest - for example, the TreeVar is probably a global and was created on the main thread, but it should be possible to start a new thread where you give it a value and then call trio.run() -- and if you do this multiple times, the different threads shouldn't conflict with each other.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I created a separate dummy task class instead so we don't create trio tasks that trio doesn't know about. 
The previous implementation is not correct because of the assumptions I made. I've addressed these 2 as well. 

@agnesnatasya
Copy link
Author

Thank you very much for the feedback!  Apologies, I definitely was only addressing a single-threaded program when implementing the previous version.

General approach for the current version: 1 'sync context task' per thread (applies to both trio thread and kernel thread). Store this task in threading.local data instead of a contextvars because contextvars behave differently in trio thread and kernel thread.

I've also implemented 2 levels of async context.

When you access a TreeVar inside a Trio run whose value was set outside, you should get the value that was set in the context where trio.run() was called. That's available using trio.lowlevel.current_root_task().context.

This is the behavior in the previous and current version. I get the value from the sync context task for that particular thread.

Let me know if I'm still missing anything!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TreeVars should work in sync code, because contextvars do
2 participants