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

Detect cycles on declaration #2

Open
numberoverzero opened this issue Jan 24, 2019 · 1 comment
Open

Detect cycles on declaration #2

numberoverzero opened this issue Jan 24, 2019 · 1 comment
Assignees

Comments

@numberoverzero
Copy link

This isn't very important because it'll fail loud and fast[0] but it might be nice to detect when provider dependencies form a cycle at declaration and raise a well-formed error instead of getting RecursionError on the first di.resolve("something-in-the-cycle") call.

For example, a complex graph could have a cycle that doesn't get pulled in until some heavy objects have already been loaded, which means the user partially creates some expensive resources and then crashes out[1].

Here's the repro[2]:

In [1]: from mainline import Di; di = Di()

In [2]: @di.f("rock")
   ...: @di.i("scissors")
   ...: def rock_factory(scissors):
   ...:     assert scissors == "Hi I'm scissors"
   ...:     return "Yep I'm rock"

In [3]: @di.f("scissors")
   ...: @di.i("paper")
   ...: def scissors_factory(paper):
   ...:     assert paper == "Whoa but I'm paper"
   ...:     return "Hi I'm scissors"

In [4]: @di.f("paper")
   ...: @di.i("rock")
   ...: def paper_factory(rock):
   ...:     assert rock == "Yep I'm rock"
   ...:     return "Whoa but I'm paper"

In [5]: try:
   ...:     di.resolve("rock")
   ...: except RecursionError:
   ...:     print("Ran out of turtles")
Ran out of turtles

[0] Someone would hit this situation what, once ever?
[1] As a highly contrived scenario, someone setting up aws resources doesn't know about the hundreds of other tools available and rolls their own.
[2] You probably didn't need this. Sorry.

@akatrevorjay
Copy link
Owner

@numberoverzero Hi, thanks for reporting this! Sorry I took so long to see it!

I agree that it would be nice to show the user a better error than to hit the recursion limit.
I've seen this during usage a small number of times myself. It could definitely be improved!

I am leery about failing hard on a user before they attempt to actually use their recursively dependent factories. The gripe I have about raiseing at declaration time is that it's not an error until it's attempted to be resolved. (It is technically possible to overwrite a provider later (say with .update(catalog)), resolving the recursive dependency chain.)

All said, I haven't been able to come up with a simple solution to this. Any ideas?

One option is to check for this at runtime (resolve) time, using a context manager to track the current stack of executing providers (contextvars would also suffice in place of this).

[2] Examples always appreciated 👍

Love your bottom library by the way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants