Context container might not be thread safe

Question

Context container might not be thread safe

Opened this issue a year ago · 6 comments

Lastly, I encountered an error while trying to use a same context container with a context manager in parallel threads. One of my dependency could not be resolved in one thread as if the definition of that dependency was no longer existing in the container.

I've made a code snippet to reproduce the issue

import random
from collections.abc import Iterable
from concurrent.futures import ThreadPoolExecutor
from lagom import (
    Container,
    ContextContainer,
    ExplicitContainer,
    context_dependency_definition,
    dependency_definition,
)

class Object:
    def __init__(self) -> None:
        self.id = random.randint(0, 9999)

class MySingleton(Object):
    pass

class MyContextSingleton(Object):
    def __init__(self, foo: str) -> None:
        super().__init__()
        self.foo = foo
    def close(self) -> None:
        print(f"closing {self.id}")

class MyDependency(Object):
    def __init__(self, ctx: MyContextSingleton) -> None:
        super().__init__()
        self.ctx = ctx

class MyService(Object):
    def __init__(self, dep: MyDependency) -> None:
        super().__init__()
        self.dep = dep

container = ExplicitContainer(log_undefined_deps=True)

@dependency_definition(container, singleton=True)
def _get_my_singleton() -> MySingleton:
    return MySingleton()

@dependency_definition(container)
def _get_my_service(c: Container) -> MyService:
    return MyService(c[MyDependency])

@dependency_definition(container)
def _get_my_dependency(c: Container) -> MyDependency:
    return MyDependency(c[MyContextSingleton])

@context_dependency_definition(container)
def _get_my_context_singleton() -> Iterable[MyContextSingleton]:
    cli = MyContextSingleton("bar")
    try:
        yield cli
    finally:
        cli.close()

context_container = ContextContainer(
    container, context_types=[], context_singletons=[MyContextSingleton]
)

def test(num: int) -> None:
    with context_container as c:
        service1 = c[MyService]
        service2 = c[MyService]
        print(
            f"Thread {num} - MySingleton {c[MySingleton].id} - MyService {service1.id} - MyDependency {service1.dep.id} - MyContextSingleton {service1.dep.ctx.id}"
        )
        print(
            f"Thread {num} - MySingleton {c[MySingleton].id} - MyService {service2.id} - MyDependency {service2.dep.id} - MyContextSingleton {service1.dep.ctx.id}"
        )

with ThreadPoolExecutor() as pool:
    f1 = pool.submit(test, 1)
    f2 = pool.submit(test, 2)
    f1.result()
    f2.result()

This snippet tries to run a same function in two parallel threads. That function is creating a context manager for our context container and pull our singleton and service. I expected to have the same singleton instance for both threads as well as an unique MyContextSingleton instance per thread (because it is a context singleton).
However It ends up with the following error

Thread 1 - MySingleton 7874 - MyService 3904 - MyDependency 922 - MyContextSingleton 989
Thread 1 - MySingleton 7874 - MyService 1379 - MyDependency 9802 - MyContextSingleton 989
closing 989
Traceback (most recent call last):
  File "lagom\container.py", line 391, in _reflection_build_with_err_handling
lagom.exceptions.UnresolvableType: Unable to construct dependency of type str The constructor probably has some unresolvable dependencies: str
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "lagom\container.py", line 392, in _reflection_build_with_err_handling
  File "lagom\container.py", line 406, in _reflection_build
  File "lagom\container.py", line 425, in _infer_dependencies
  File "lagom\container.py", line 265, in resolve
  File "lagom\container.py", line 395, in _reflection_build_with_err_handling
lagom.exceptions.UnresolvableType: Unable to construct dependency of type str The constructor probably has some unresolvable dependencies: str
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "c:\Users\TBEBGOD\Developement\My-project\snippet.py", line 93, in <module>
    f2.result()
  File "C:\Users\TBEBGOD\.pyenv\pyenv-win\versions\3.10.11\lib\concurrent\futures\_base.py", line 458, in result
    return self.__get_result()
  File "C:\Users\TBEBGOD\.pyenv\pyenv-win\versions\3.10.11\lib\concurrent\futures\_base.py", line 403, in __get_result
    raise self._exception
  File "C:\Users\TBEBGOD\.pyenv\pyenv-win\versions\3.10.11\lib\concurrent\futures\thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
  File "c:\Users\TBEBGOD\Developement\My-project\snippet.py", line 78, in test
    service1 = c[MyService]
  File "lagom\container.py", line 381, in __getitem__
  File "lagom\container.py", line 259, in resolve
  File "lagom\definitions.py", line 37, in get_instance
  File "c:\Users\TBEBGOD\Developement\My-project\snippet.py", line 54, in _get_my_service
    return MyService(c[MyDependency])
  File "lagom\container.py", line 381, in __getitem__
  File "lagom\container.py", line 259, in resolve
  File "lagom\definitions.py", line 37, in get_instance
  File "c:\Users\TBEBGOD\Developement\My-project\snippet.py", line 59, in _get_my_dependency
    return MyDependency(c[MyContextSingleton])
  File "lagom\container.py", line 381, in __getitem__
  File "lagom\container.py", line 265, in resolve
  File "lagom\container.py", line 395, in _reflection_build_with_err_handling
lagom.exceptions.UnresolvableType: Unable to construct dependency of type MyContextSingleton The constructor probably has some unresolvable dependencies: MyContextSingleton

The first thread shows the container is working as expected, in particular the two services resolved ends up with the same MyContextSingleton. The second shows that the container level singleton is working as expected because we got the same instance in both threads. However, the second thread crash when trying to resolve a MyContextSingleton.

It feels like on the second thread the definition for MyContextSingleton no longer exist causing the container to use reflection to resolve it (while it should not) which obviously fails because it can not resolve the foo: str dependency.
Note that, if I clone the container before opening the context, it works as expected (with context_container.clone() as c:.
If I run the two threads serially it works as well.

Answer 1 · 2023-09-08T07:53:46.000Z

This is a bit concerning. Let me take a look at this. Thank you for providing the snippet

Answer 2 · 2023-09-18T18:00:05.000Z

Now that I think about this a little more I don't think this is a good design. I can't actually see how it could ever be threadsafe. The context_container in with context_container as c: is always going to be shared between threads with this design and that's a problem. I think I'll fix it by changing to one of these interfaces but it will be a braking change.

with context_container(container, context_types=[], context_singletons=[MyContextSingleton]) as c:
   c[MyService]

or

context_container = ContextContainer(
    container, context_types=[], context_singletons=[MyContextSingleton]
)
# ... later
with context_container.prepare() as c:
    service1 = c[MyService]

Answer 3 · 2023-09-18T18:40:58.000Z

Why no simply always call the clone method each time the context container is entered ? Because this actually works, it’s my actual workaround and my tests shows that using one container clone per thread is fine. The current issue IMO is that the current enter method behavior is to reuse the current container (return self) if the container is currently entered (I.e has an exit stack) while returning a clone in the other case.

But maybe there is something more complicated I’m missing.

Answer 4 · 2023-09-19T06:05:45.000Z

@g0di I tried that as a solution last night and that's when I realised the current solution is dependent on the original (not cloned) container's exit method to tidy up. If there's more than one clone then you don't know which clone to tidy up on exit. This problem is solved if you clone before invoking enter. Which sadly I think is imposible with the current interface.

Answer 5 · 2023-09-19T18:50:25.000Z

Well, you know the code better than me so I trust you. About your proposals I think the first one is a bad idea because it would require to « setup » the context container many times contrarily to the second proposal.

However, I don’t really get the difference between the « prepare » method and the actual « clone » one then since as you mentioned, cloning before entering works fine

Answer 6 · 2023-09-20T14:37:01.000Z

@g0di the main reason I would want to add the prepare method is to make sure that it can't be accidentally used without calling .clone(). So in addition to adding the prepare method I would remove the enter and exit from the class so that it cannot be directly used as a context manager without a call to prepare.

For anyone reading this issue there is a work around for now which is the following:

def _example_function(num: int) -> None:
    with context_container.clone() as c:
        service1 = c[MyService]
        service2 = c[MyService]
        print(
            f"Thread {num} - MySingleton {c[MySingleton].id} - MyService {service1.id} - MyDependency {service1.dep.id} - MyContextSingleton {service1.dep.ctx.id}"
        )
        print(
            f"Thread {num} - MySingleton {c[MySingleton].id} - MyService {service2.id} - MyDependency {service2.dep.id} - MyContextSingleton {service1.dep.ctx.id}"
        )

the important part is the .clone() as this prevents the threads from sharing a container.