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

asyncio contextvars are not assigned separate contexts? #1656

Closed
hashbrowncipher opened this issue Jul 3, 2020 · 6 comments
Closed

asyncio contextvars are not assigned separate contexts? #1656

hashbrowncipher opened this issue Jul 3, 2020 · 6 comments
Labels
PyVer: python3 Affects Python 3 Status: cantfix Something that cannot be changed in gevent Type: Question User support and/or waiting for responses

Comments

@hashbrowncipher
Copy link
Contributor

hashbrowncipher commented Jul 3, 2020

  • gevent version: 20.6.2
  • Python version: cPython 3.7.6
  • Operating System: Ubuntu Focal

Description:

The following program behaves differently when monkeypatched, compared to when running under the Python stdlib. Using the stdlib contextvars, each asyncio task prints its own chosen uuid. When monkeypatched, all tasks print the same uuid after the first loop iteration.

What I've run:

import asyncio
from contextvars import ContextVar
from uuid import uuid4

test_var = ContextVar("test")


async def printer():
    test_var.set(uuid4())
    while True:
        print(test_var.get())
        await asyncio.sleep(0.1)


async def launch():
    ret = []
    for i in range(4):
        ret.append(asyncio.create_task(printer()))
        await asyncio.sleep(0.025)
    return ret


async def main():
    printers = await launch()
    await asyncio.gather(*printers)


if __name__ == '__main__':
    asyncio.run(main())
@hashbrowncipher
Copy link
Contributor Author

hashbrowncipher commented Jul 3, 2020

stdlib:

$ python test_contextvars.py
a2268100-191b-4cec-b50e-6d9979e5b6d6
78afb085-7304-4b01-a9eb-30caa766f23a
b3da0551-a4e0-4e50-a3c3-1d4155655663
1f830962-83ce-45ec-b95a-611353b868e9
a2268100-191b-4cec-b50e-6d9979e5b6d6
78afb085-7304-4b01-a9eb-30caa766f23a
b3da0551-a4e0-4e50-a3c3-1d4155655663
1f830962-83ce-45ec-b95a-611353b868e9

monkeypatched:

$ python -m gevent.monkey test_contextvars.py
9e9531cd-ee6e-49a6-be17-b205beff5728
b5264a7a-257d-42ef-aad6-308284badc2e
8e776afb-97b6-481f-a083-d0f01873917e
4911d6da-6c30-47e8-91bc-e25c6e594938
4911d6da-6c30-47e8-91bc-e25c6e594938
4911d6da-6c30-47e8-91bc-e25c6e594938
4911d6da-6c30-47e8-91bc-e25c6e594938
4911d6da-6c30-47e8-91bc-e25c6e594938
```

@jamadden
Copy link
Member

jamadden commented Jul 3, 2020

gevent's contextvars work with gevent, not with asyncio. Just as asyncio's contextvars are not greenlet aware, gevent's contextvars are not asyncio-task aware. You generally can't mix-and-match the two.

@jamadden jamadden added PyVer: python3 Affects Python 3 Status: cantfix Something that cannot be changed in gevent Type: Question User support and/or waiting for responses labels Jul 3, 2020
@hashbrowncipher
Copy link
Contributor Author

hashbrowncipher commented Jul 5, 2020

Thanks for the clarification. Having now looked at both implementations, I think I see what you mean: melding the two would be hard to do from within gevent. We'd basically have to instrument every switch operation, storing the stack (linked-list, really) of PyContext objects from the interpreter state every time we switch into a new greenlet. So far as I can tell, there's no clean way to access the requisite information from within the Python interpreter, but a C extension with access to the PyThreadState object would make it straightforward.

I started looking at the greenlet library, and I realized that adding support for switching contextvars becomes straightforward as a patch within that codebase. It also makes sense that the functionality be implemented there: greenlet is meant to facilitate switching interpreter state in and out, and contextvars are implemented as an extension of the interpreter state. To that end, I'm thinking to send this branch as a PR upstream. Would you be interested in taking a look at that change and providing feedback?

@jamadden
Copy link
Member

jamadden commented Jul 6, 2020

I think I like the idea very much, and the basic implementation looks reasonable to me at a glance.

I'm a wee bit concerned, though, that this makes every greenlet its own "context" (if I understand correctly). It's sort of like an implicit monkey-patch. It would make certain patterns impossible (such as using two greenlets from a single asyncio task). I don't know how upstream will feel about that.

@hashbrowncipher
Copy link
Contributor Author

hashbrowncipher commented Jul 6, 2020

You're thinking of a situation where one asyncio task launches two greenlets, and they want to be able to call .set(value) on a ContextVar instance, and see each others' changes? I think the endorsed pattern there is for the launching code to call .set(dict()), and then launch two greenlets with copy_context().run. The two greenlets will then each fetch the dict using .get() on the ContextVar instance, and communicate through mutations of the shared dictionary object.

@hashbrowncipher
Copy link
Contributor Author

hashbrowncipher commented Jul 7, 2020

When I build gevent against a patched greenlet egg, the cython runtime loader emits this warning: greenlet.greenlet size changed, may indicate binary incompatibility, and my process segfaulted shortly thereafter. I have updated the PR upstream to put the new attribute at the end of the struct. I believe this avoids re-arranging fields incompatibly, but the warning remains. I think that to be safe against this kind of breakage, gevent may need to vendor greenlet, like it does libev and c-ares, or specify an exact version pin. But I'm guessing you've seen this before, based on this convenient comment:

191 greenlet_requires = [
192     # We need to watch our greenlet version fairly carefully,
193     # since we compile cython code that extends the greenlet object.
194     # Binary compatibility would break if the greenlet struct changes.
195     # (Which it did in 0.4.14 for Python 3.7)
196     'greenlet >= 0.4.16; platform_python_implementation=="CPython"',
197 ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PyVer: python3 Affects Python 3 Status: cantfix Something that cannot be changed in gevent Type: Question User support and/or waiting for responses
Projects
None yet
Development

No branches or pull requests

2 participants