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

fail on accidential recursive usage due to lock deadlocking in same thread #317

Open
RonnyPfannschmidt opened this issue Nov 29, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@RonnyPfannschmidt
Copy link

in a workaround codepath i used a sub-container in a provider

due to a oversight the tests never triggered the bit where requesting it may deadlock by locking the parent container again

ideally the Locks would fail directly in case the same thread requests them

my local workaround was to use a Rlock until the bad codepath can be refactored not to need a sub-container

minimal reproducer will follow

@Tishka17 Tishka17 added the bug Something isn't working label Nov 29, 2024
@Tishka17
Copy link
Member

I guess, we need to pass here special container adapter which calls _get_unlocked directly without any locks here.

@RonnyPfannschmidt
Copy link
Author

Rlock is a reasonable hack

Id strongly recommend warning as it is a code smell that shouldn't be enabled easily and without a note

Silent Deadlocked is bad

But this shouldn't ever work without a warning

@Tishka17
Copy link
Member

Tishka17 commented Nov 30, 2024

Idea
When user requests a Container instance, pass instead a special object:

class FakeContainer:
   def get(self, typ, component):
      return self.container._get_unlocked(DependencyKey(typ, compoenent))

It is important to do it only within provider

@Tishka17
Copy link
Member

relates to #9

@RonnyPfannschmidt
Copy link
Author

It's absolutely shouldn't

That's hiding serious errors and won't handle passover

@RonnyPfannschmidt
Copy link
Author

import threading
from contextlib import closing
from typing import NewType

from dishka import (
    Container,
    Provider,
    Scope,
    from_context,
    make_container,
    provide,
)

EnvName = NewType("EnvName", str)

Dependent = NewType("Dependent", str)

Sub = NewType("Sub", str)


Mess = NewType("Mess", str)


class NestingFunProvider(Provider):
    envname = from_context(EnvName, scope=Scope.APP)

    @provide(scope=Scope.APP)
    def dependent(self, env: EnvName) -> Dependent:
        return Dependent(EnvName)

    @provide(scope=Scope.REQUEST)
    def sub(self, dep: Dependent) -> Sub:
        return Sub(dep)

    @provide(scope=Scope.APP)
    def mess(self, appc: Container) -> Mess:
        with appc() as reqc:
            return Mess(reqc.get(Sub))


# the lock should warn
c = make_container(
    NestingFunProvider(),
    context={EnvName: "stage"},
    lock_factory=threading.RLock,
)
with closing(c):
    c.get(Mess)

is the code example where i ran into the issue - withRLock it passes

however rlock is also always a indication of a latent bug

i'd recommend to have a rlock that warns as default - same for the async ones
i condsider it critical that the container warns

in my real world example i was abusing the request scope to obtain data where i need a while to implement the app scope variant propperly

@Tishka17
Copy link
Member

I don't think it is always a problem with user code, ias for me it is only a bug in library. Provider code is valid, though it might be not perfect, we should be able to deal with it

@RonnyPfannschmidt
Copy link
Author

My point is the code is actually not correct

Which is why warning is critical when enabling this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants