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

How to reset ContextVar in separate sync_to_async calls #267

Open
adamantike opened this issue Jun 4, 2021 · 3 comments
Open

How to reset ContextVar in separate sync_to_async calls #267

adamantike opened this issue Jun 4, 2021 · 3 comments

Comments

@adamantike
Copy link

adamantike commented Jun 4, 2021

After filing https://code.djangoproject.com/ticket/32815, it was more evident that the issue I'm experiencing when trying to reset a ContextVar is related to asgiref. However, I'm not sure about this being a bug or an implementation issue, so any feedback is appreciated.

This issue has been raised in our efforts to add Django ASGI support to OpenTelemetry (open-telemetry/opentelemetry-python-contrib#391). OpenTelemetry uses a Django middleware class to implement both process_request and process_response methods. As both of them are synchronous, they are converted with sync_to_async and called separately (with thread_sensitive=True). The first call sets the ContextVar value, and persists the token to reset it. The second call uses the persisted token to reset the ContextVar to its previous value.


Tests added to tests/test_sync_contextvars.py, which reproduce the issue:

@pytest.mark.asyncio
async def test_sync_to_async_contextvar_reset():
    """
    Tests a ContextVar can be set and reset in different calls to sync_to_async.
    """
    # Define set and reset functions
    def sync_set_function():
        assert foo.get() == "bar"
        token = foo.set("baz")
        return token

    def sync_reset_function(token):
        assert foo.get() == "baz"
        foo.reset(token)
        return 42

    foo.set("bar")
    async_set_function = sync_to_async(sync_set_function)
    token = await async_set_function()
    async_reset_function = sync_to_async(sync_reset_function)
    assert await async_reset_function(token) == 42
    assert foo.get() == "bar"


def test_async_to_sync_contextvar_reset():
    """
    Tests a ContextVar can be set and reset in different calls to async_to_sync.
    """
    # Define set and reset functions
    async def async_set_function():
        await asyncio.sleep(1)
        assert foo.get() == "bar"
        token = foo.set("baz")
        return token

    async def async_reset_function(token):
        await asyncio.sleep(1)
        assert foo.get() == "baz"
        foo.reset(token)
        return 42

    foo.set("bar")
    sync_set_function = async_to_sync(async_set_function)
    token = sync_set_function()
    sync_reset_function = async_to_sync(async_reset_function)
    assert sync_reset_function(token) == 42
    assert foo.get() == "bar"

Test traceback:

tests/test_sync_contextvars.py:79:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
asgiref/sync.py:443: in __call__
    ret = await asyncio.wait_for(future, timeout=None)
/usr/lib/python3.9/asyncio/tasks.py:442: in wait_for
    return await fut
/usr/lib/python3.9/concurrent/futures/thread.py:52: in run
    result = self.fn(*self.args, **self.kwargs)
asgiref/sync.py:484: in thread_handler
    return func(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

token = <Token var=<ContextVar name='foo' at 0x7f70119bf220> at 0x7f70119bbf40>

    def sync_reset_function(token):
        assert foo.get() == "baz"
>       foo.reset(token)
E       ValueError: <Token var=<ContextVar name='foo' at 0x7f70119bf220> at 0x7f70119bbf40> was created in a different Context
@andrewgodwin
Copy link
Member

What would you expect to happen - that everything shares the same context (both in and outside the sync_to_async/async_to_sync calls)? I believe that is what the intended effect is, but I didn't author the contextvar code in asgiref so I'd have to go re-learn it to see where this might be.

@adamantike
Copy link
Author

Thanks for following up, Andrew! :)

Yes, I think that's what I would expect. This is the simplest test I found to reproduce the issue:

import contextvars

from asgiref.sync import sync_to_async
import pytest

foo = contextvars.ContextVar("foo")

@pytest.mark.asyncio
async def test_sync_to_async_contextvar_reset():
    token = foo.set("bar")
    await sync_to_async(foo.reset)(token)
> /mnt/data/Proyectos/third_party/asgiref/asgiref/sync.py(423)__call__()
    422             context = contextvars.copy_context()
--> 423             child = functools.partial(self.func, *args, **kwargs)
    424             func = context.run

ipdb> !context
<Context object at 0x7fcf31388f00>
ipdb> contextvars.copy_context()
<Context object at 0x7fcf30a76700>
ipdb> context2 = contextvars.copy_context()
ipdb> !context
<Context object at 0x7fcf31388f00>
ipdb> !context2
<Context object at 0x7fcf30a76700>
ipdb> contextvars.copy_context()
<Context object at 0x7fcf31394a40>

@andrewgodwin
Copy link
Member

Hm, interesting. I suspect whoever initially wrote this didn't test the reset() usage, then, and instead were just trying to persist values inside.

Unless you want to dive into this and see if you can make it work "as intended", I'll have to see if I can get around to looking at this later in the month (I'm mostly taking a break from OSS at the moment) - at the very worst, I'd hope we can at least get better errors and document the intended behaviours.

adamantike added a commit to adamantike/opentelemetry-python-contrib that referenced this issue Jun 5, 2021
New-style middlewares were [introduced][django_1_10_changelog] in Django
`1.10`, and also `settings.MIDDLEWARE_CLASSES` was
[removed][django_2_0_changelog] in Django 2.0.

This change migrates the Django middleware to conform with the new
style. This is useful because it will help us solve the pending issue in
open-telemetry#391.

By having a single entrypoint to the middleware, `__call__`, which is
[wrapped with `sync_to_async` just once][call_wrapped] for async
requests, we avoid the [issue][asgiref_issue] where a `ContextVar`
cannot be reset from a different context.

With the current deprecated `MiddlewareMixin` way, both `process_request`
and `process_response` were being
[wrapped separately with `sync_to_async`][mixin_wrapped], which was the
source of the mentioned issue.

[django_1_10_changelog]: https://docs.djangoproject.com/en/3.2/releases/1.10/#new-style-middleware
[django_2_0_changelog]: https://docs.djangoproject.com/en/3.2/releases/2.0/#features-removed-in-2-0
[call_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/core/handlers/base.py#L54-L57
[mixin_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/utils/deprecation.py#L137-L147
[asgiref_issue]: django/asgiref#267
adamantike added a commit to adamantike/opentelemetry-python-contrib that referenced this issue Jun 5, 2021
New-style middlewares were [introduced][django_1_10_changelog] in Django
`1.10`, and also `settings.MIDDLEWARE_CLASSES` was
[removed][django_2_0_changelog] in Django 2.0.

This change migrates the Django middleware to conform with the new
style. This is useful because it will help us solve the pending issue in
open-telemetry#391.

By having a single entrypoint to the middleware, `__call__`, which is
[wrapped with `sync_to_async` just once][call_wrapped] for async
requests, we avoid the [issue][asgiref_issue] where a `ContextVar`
cannot be reset from a different context.

With the current deprecated `MiddlewareMixin` way, both `process_request`
and `process_response` were being
[wrapped separately with `sync_to_async`][mixin_wrapped], which was the
source of the mentioned issue.

[django_1_10_changelog]: https://docs.djangoproject.com/en/3.2/releases/1.10/#new-style-middleware
[django_2_0_changelog]: https://docs.djangoproject.com/en/3.2/releases/2.0/#features-removed-in-2-0
[call_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/core/handlers/base.py#L54-L57
[mixin_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/utils/deprecation.py#L137-L147
[asgiref_issue]: django/asgiref#267
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

No branches or pull requests

2 participants