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

separate sync_container_context to fix problems with the same contextvar token for every context #93

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

lesnik512
Copy link
Member

separate sync_container_context to fix problems with the same contextvar token for every context

Copy link

codecov bot commented Sep 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
tests/providers/test_context_resources.py 100.00% <100.00%> (ø)
that_depends/__init__.py 100.00% <100.00%> (ø)
that_depends/providers/__init__.py 100.00% <ø> (ø)
that_depends/providers/context_resources.py 100.00% <100.00%> (ø)

@lesnik512
Copy link
Member Author

@alexanderlazarev0 Hi) there is some problem with new container context.

The problem is that contextvar token variable is the same for all requests.

To fix this I had to separate sync container context

@lesnik512 lesnik512 merged commit ad3a43f into main Sep 28, 2024
5 checks passed
@lesnik512 lesnik512 deleted the bugfix/closing-context-resources branch September 28, 2024 08:38
@alexanderlazarev0
Copy link
Collaborator

@lesnik512
Do you have a minimal example to reproduce?

@lesnik512
Copy link
Member Author

@alexanderlazarev0 not right now, in the evening I will make an example

@lesnik512
Copy link
Member Author

lesnik512 commented Sep 28, 2024

@alexanderlazarev0 Error was the following:
RuntimeError
<Token used var=<ContextVar name='CONTAINER_CONTEXT' at 0x7fade72ab8d0> at 0x7fade06e8c80> has already been used once

File "that_depends/providers/context_resources.py", line 96, in _async_inner
async with self:
File "that_depends/providers/context_resources.py", line 89, in aexit
_CONTAINER_CONTEXT.reset(self._context_token)

Trying to build small reproducible example now

@alexanderlazarev0
Copy link
Collaborator

@lesnik512 Yes I was trying to reproduce this, but wasn't able to for now. Is this with fastapi?

@lesnik512
Copy link
Member Author

lesnik512 commented Sep 28, 2024

@alexanderlazarev0 it's in kafka consumer.

There is a method which called on each message from topic:

    @container_context(build_di_context("write"))
    async def process_one_message(
        self,
        message,
        _: dict[str, typing.Any] | None = None,
    ) -> None:
         ...

Theres is also another version of this error:

ValueError
<Token var=<ContextVar name='CONTAINER_CONTEXT' at 0x7f3098d479c0> at 0x7f30807b7bc0> was created in a different Context

@alexanderlazarev0
Copy link
Collaborator

alexanderlazarev0 commented Sep 28, 2024

@lesnik512 Okay seems the __exit__/__aexit__ method of the containter_context is being called twice by different actors.

@lesnik512
Copy link
Member Author

lesnik512 commented Sep 28, 2024

@alexanderlazarev0 I think, the problem is occured when decorator used: container_context initialized only once and _context_token is shared between different contexts

@alexanderlazarev0
Copy link
Collaborator

alexanderlazarev0 commented Sep 28, 2024

@lesnik512 Ah! I got it I think, its a 2 liner:

# container_context class
 def __call__(self, func: typing.Callable[P, T]) -> typing.Callable[P, T]:
        if inspect.iscoroutinefunction(func):

            @wraps(func)
            async def _async_inner(*args: P.args, **kwargs: P.kwargs) -> T:
                async with self: # <- HERE
                    return await func(*args, **kwargs)  # type: ignore[no-any-return]

            return typing.cast(typing.Callable[P, T], _async_inner)

        @wraps(func)
        def _sync_inner(*args: P.args, **kwargs: P.kwargs) -> T:
            with self: # <-- HERE
                return func(*args, **kwargs)

        return _sync_inner

This should fix it:

 def __call__(self, func: typing.Callable[P, T]) -> typing.Callable[P, T]:
        if inspect.iscoroutinefunction(func):

            @wraps(func)
            async def _async_inner(*args: P.args, **kwargs: P.kwargs) -> T:
                async with container_context():
                    return await func(*args, **kwargs)  # type: ignore[no-any-return]

            return typing.cast(typing.Callable[P, T], _async_inner)

        @wraps(func)
        def _sync_inner(*args: P.args, **kwargs: P.kwargs) -> T:
            with container_context():
                return func(*args, **kwargs)

        return _sync_inner

This is still on that_depends==1.19.3

@lesnik512
Copy link
Member Author

lesnik512 commented Sep 28, 2024

@alexanderlazarev0 seems, like this could work.

But _initial_context must be passed, I think, in __call__:

container_context(self._initial_context)

@lesnik512
Copy link
Member Author

@alexanderlazarev0 But still I have no idea how to reproduce this in tests)

@alexanderlazarev0
Copy link
Collaborator

@lesnik512 I think you might be able to reproduce it by calling an @container_context decorated function twice in the same call stack (i need to try just my first guess)

@alexanderlazarev0 seems, like this could work.

But _initial_context must be passed, I think, in __call__:

container_context(self._initial_context)

Yes you are right this will need to be added.

I will let you know if i can reproduce the issue.

@alexanderlazarev0
Copy link
Collaborator

alexanderlazarev0 commented Sep 28, 2024

@lesnik512 Yes got It and the suggest fix works!
Error example:

from that_depends import providers
from that_depends.container import BaseContainer
from that_depends.injection import inject
from that_depends.providers.context_resources import container_context


def context_resource_creator():
    yield "interesting"


class MyContainer(BaseContainer):
    context_resource = providers.ContextResource(context_resource_creator)
@container_context()
@inject
def some_injected(depth, val = MyContainer.context_resource):
    if depth ==2:
        return val
    return some_injected(depth+1)


if __name__ == "__main__":
    some_injected(1) # RuntimeError: <Token used var=<ContextVar name='CONTAINER_CONTEXT' at 0x10497e930> at 0x1049b2180> has already been used once

@alexanderlazarev0
Copy link
Collaborator

@lesnik512 So the question is really then just one of taste:

  1. Keep the current implementation, but now you need to actively annotate correctly with either sync_container_context or container_context
  2. Revert to the old one which is less verbose and arguably easier to use for annotations (less thought required), however it is perhaps not as intuitive to use outside annotationswith container_context() and async with container_context().
  3. Theoretically, both solutions can coexist, but I think 1 clear way to do things is preferred.

@lesnik512
Copy link
Member Author

@alexanderlazarev0 Nicely done! So I reverted to the previous version, applied fix and wrote some tests #94. Please, take a look

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.

2 participants