Add the ability to modify a greenlet's contextvars context

[oremanj]

This PR adds gr_context as a writable greenlet attribute as suggested in the comments of #183. Unlike my proposal in #196, it is fully backward-compatible. I still think the default that was chosen in 0.4.17 is unfortunate, but I suspect this PR will require less bikeshedding than #196, and it allows those who wish to replicate the pre-0.4.17 behavior to do so.

I chose to make gr_context accessible and modifiable even for the greenlet that is currently running in your thread (though not for greenlets that are running in other threads), because that seemed far less confusing than the alternative. This effectively makes it a direct accessor/mutator for PyThreadState::context, which is not otherwise exposed to Python code. I think that's OK, since that value is just going to be stored in PyGreenlet::context later anyhow -- modifying gr_context of the running greenlet doesn't let you do anything you couldn't do by modifying gr_context of a non-running greenlet. Using either ability carelessly can cause the exit from Context.run() to raise an exception, but nothing worse than that AFAICT.

[oremanj]

I didn't actually look at the pseudocode in #183 while I was writing this, so there are some semantic differences:

• In this PR gr_context doesn't exist if greenlet doesn't have contextvars support, rather than always returning None.
• I chose to make del gr.gr_context behave the same as gr.gr_context = None, rather than raising an error.

I'm happy to change either of these if you prefer the approach that was sketched in #183. (I didn't add a constructor argument either, but I figure that can be done in a separate PR if it's desired.)

[jamadden]

Thanks very much for this!

I obviously like this basic approach, but I'm still wrapping my head around the issue(s), and it would be nice to get some more feedback from those involved in sqlalchemy/sqlalchemy#5615 and #183 (I'll solicit that feedback).

I do have some responses to your comments:

In this PR gr_context doesn't exist if greenlet doesn't have contextvars support, rather than always returning None.

What I was trying to do with that suggestion was militate against the fact that greenlets have __dict__ and support arbitrary attributes. Reading or assigning to gr_context in a greenlet built without context support would still appear to "work", but wouldn't actually do the right thing.

I expect consumers like sqlalchemy that want to inherit the context after spawning to write code like this:

glet = greenlet(…)
glet.gr_context = get_inherited_context()
…

Since "errors should never pass silently," I don't think that should work on versions of greenlet without context support. I think (having thought a bit more 😄) that it should always raise AttributeError.

(Because of the use of a compile-time-define that can override the default compile-time-Python-version check, one can't assume that if you're running on Python 3.7+ greenlet will have context support. Because they're not documented, I doubt many people would be aware of the fact that the compile-time-define is exposed in the greenlet module. Ultimately I'd like to get away from customizable compile-time-defines overriding the defaults so that the ABI is more reliable…)

Because of the way greenlet happens to be implemented on CPython, one could check for the attribute on the type of the greenlet:

glet = greenlet(…)
if hasattr(type(glet), 'gr_context'):
    glet.gr_context = get_inherited_context()
…

But no one does that. They might check the instance, but that breaks down if anyone forgets to do the check and just assigns (because it goes in the __dict__). Also, checking the type breaks if greenlet is implemented differently, as it is in PyPy. (On CPython, hasattr(greenlet.greenlet, 'parent') == True, but on PyPy, hasattr(greenlet.greenlet, 'parent') == False.)

So to be safe, I think gr_context needs to always be around. On versions without context support, it should raise an AttributeError for all operations. If they wanted to support versions of greenlet without gr_context, consumers would be forced to write:

glet = greenlet(…)
try:
    glet.gr_context = get_inherited_context()
except AttributeError:
    pass

It's clearly not perfect because it doesn't work on older versions of greenlet, but I don't know what to do about that. Checking the type isn't portable, checking the module compile-time-define constant isn't documented, and checking the __version__ doesn't work (because of the override ability), plus the fact that it's usually frowned upon for feature detection.

I didn't add a constructor argument either,

I'm fine with that. A constructor argument is partial mitigation against what I described above, but it's not a complete solution either.

I chose to make del gr.gr_context behave the same as gr.gr_context = None, rather than raising an error.

Those seem like they should be two distinct operations to me. It's not clear why they should be synonyms. (It's always easier to relax the behaviour later if necessary.)

[zzzeek]

hey there -

I'm not deeply familiar with contextvars as of yet but can you clarify what the source of get_inherited_context() would be?

[jamadden]

I'm not deeply familiar with contextvars as of yet but can you clarify what the source of get_inherited_context() would be?

contextvars.copy_context, probably, depending on the exact use case (the use cases for this are not something I claim to fully understand).

[zzzeek]

I guess that's accurate. @fantix can you sign off on that approach ?

[fantix]

This PR looks good to me, thanks for getting this done!

@zzzeek I think this is good enough and I would be able to update sqlalchemy/sqlalchemy#5616 to use this PR in a cleaner way.

[CaselIT]

I agree with the @zzzeek and @fantix

(the use cases for this are not something I claim to fully understand).

The use case is basically to use greenlets to execute async code from a sync caller. More context is here #173 and https://gist.github.com/snaury/202bf4f22c41ca34e56297bae5f33fef

[oremanj]

Thanks for merging! I was just looking at this again and I think the notes I wrote in the docs about gr_context being safer than Context.run() are partially specious: test_break_ctxvars does pass despite the name. gr_context assignment still provides important capabilities, and it's still potentially dangerous to assign it when the greenlet is inside a call to Context.run(), but I don't think it's necessary in general to prefer gr = greenlet(target); gr.gr_context = foo; gr.switch() over gr = greenlet(foo.run); gr.switch(target) on correctness grounds.

I'll work on a docs PR to fix this, but it might not be for a few days.