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

Synchronous setup and teardown for @value decorator. #132

Closed
proofit404 opened this issue Jul 25, 2019 · 9 comments · Fixed by #544
Closed

Synchronous setup and teardown for @value decorator. #132

proofit404 opened this issue Jul 25, 2019 · 9 comments · Fixed by #544

Comments

@proofit404
Copy link
Owner

proofit404 commented Jul 25, 2019

Value decorator should handle generators.

This is very similar to the contextlib.contextmanager.

class Container(Injector):
    app = App

    @value
    def db_connection(pool):
        connection = pool.aquire()
        yield connection
        connection.release()

with Container as initialized:
    initialized.app.process()
@thedrow
Copy link
Contributor

thedrow commented Jul 26, 2019

As it turns out contextlib.AsyncExitStack can do all the cleanup job for us.
All we need to do is register all context managers in it.

@proofit404 proofit404 removed the asyncio label Feb 4, 2020
@proofit404 proofit404 added this to the Setup and teardown. milestone Jan 20, 2021
@proofit404 proofit404 changed the title Support setup and teardown processes using @value and context manager. Synchronous setup and teardown for @value decorator. Jan 20, 2021
@mastern2k3
Copy link

Hey @proofit404 , I'm currently window shopping for a DI package for our project and ended up really falling for dependencies, it hits the right balance of brevity and practicality that I need.

Handling teardowns is an important part for me (closing database scopes and closing cache processes) and it looks like the feature described in this ticket is exactly what i need.

Seeing that this is not yet implemented, is there a way to achieve something similar with the current latest version?
e.g.
describe teardown logic for a dependency in some way and have it run when the context scope ends?

with Container as initialized:
    initialized.app.process()

@proofit404
Copy link
Owner Author

proofit404 commented May 10, 2022

Good morning,

Thanks for the interest in the dependencies library!

The very first step for this issue to be implemented was done some time ago. You can use Sticky scopes already. It would hold same instances within created scope, so if you call teardown after process manually, it will close right connections.

I could suggest to look at handling database transactions example from stories library. It shows how you could implement OOP decorator pattern. In that case wrapped process method would manage connections as well.

Also, open and close connections frequently may be not the best idea for performance. You could inject an instance of connection pool in your service objects, so connection management would be hidden in the single place.

Last but not least, all complicated part in that milestone relates to the asynchronous implementation. If you need synchronous context only, it maybe worth to implement before you add dependencies to your project.

Hope that helps, feel free to ask more questions if something still not clear for you.

Have a good day 🌴 🍸

Best regards,
Artem.

proofit404 added a commit that referenced this issue Jun 3, 2022
proofit404 added a commit that referenced this issue Jun 3, 2022
github-actions bot pushed a commit that referenced this issue Jun 3, 2022
# 7.3.0 (2022-06-03)

### Features

* setup and teardown of [@value](https://github.com/value) decorator [#132](#132) df62d78
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

🎉 This issue has been resolved in version 7.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@proofit404
Copy link
Owner Author

@mastern2k3 please take a look at the feature. I would appreciate feedback on how it works and if you find any issues with it, just let me know.

Thanks.

@mastern2k3
Copy link

Thanks for the ping @proofit404 , this looks perfect.
Is it correct to assume that the behavior is the same for non scoped instantiations? e.g:

initialized.app.process()
# connection.release() called after process() ends

@proofit404
Copy link
Owner Author

Unfortunately, no.

Due to the Python language semantic we could understand only Container.app, not .process().

I think we need explicitly deny Container.app, if App depends on value object generator function.

I'll create a separate ticket with verbose explanation.

@mastern2k3
Copy link

@proofit404, I dove a little deeper and found some more things that I miss.

1. Teardown ordering

Take for example this container and dependencies:

from dataclasses import dataclass
from dependencies import Injector, value


@dataclass
class One:
    def close(self):
        pass

@dataclass
class Two:
    one: One
    def close(self):
        pass

@dataclass
class Three:
    two: Two
    def close(self):
        pass

@dataclass
class Thing:
    three: Three


class SomeContainer(Injector):

    thing = Thing

    @value
    def one():
        print("before one")
        yield One()
        print("after one")

    @value
    def two(one: One):
        print("before two")
        yield Two(one=one)
        print("after two")

    @value
    def three(two: Two):
        print("before three")
        yield Three(two=two)
        print("after three")


with SomeContainer as container:
    thing = container.thing
    print(f"{thing=}")

This code will print the following:

before one
before two
before three
thing=Thing(three=Three(two=Two(one=One())))
after one
after two
after three

I noticed the dependencies are instantiated in the order of necessity (i.e. Thing needs Three which needs Two which needs One which gets instantiated first), but, on shutdown they are executed by the same order.

I think a reverse order is a more sensible default, or at least, a more commonly desired behavior.
In my example we had Three depending on Two which depends on One.
If we teardown One or Two before Three we might result in a broken state since it is assumed that Three requires Two still-up to function.

A different example could be a set of Connection and Transaction classes, where Transaction requires an initialized Connection to create and function.

In this scenario, we will teardown Connection before we close Transaction which could result in a broken state.

2. Teardown resiliency

If an exception occurs in a dependency's teardown code, for example, like so:

    @value
    def two(one: One):
        print("before two")
        yield Two(one=one)
        raise Exception("ha!")
        print("after two")

The teardown process will cut mid-way:

before one
before two
before three
thing=Thing(three=Three(two=Two(one=One())))
after one
Traceback (most recent call last):
  File "main2.py", line 53, in <module>
    print(f"{thing=}")
  File "/home/nitzan/Projects/test-depednencies/.venv/lib/python3.8/site-packages/_dependencies/injector.py", line 39, in __exit__
    cls.__context_stack__.remove()
  File "/home/nitzan/Projects/test-depednencies/.venv/lib/python3.8/site-packages/_dependencies/stack.py", line 15, in remove
    enclose.after()
  File "/home/nitzan/Projects/test-depednencies/.venv/lib/python3.8/site-packages/_dependencies/enclose.py", line 18, in after
    callback()
  File "/home/nitzan/Projects/test-depednencies/.venv/lib/python3.8/site-packages/_dependencies/objects/value.py", line 72, in __call__
    next(self.generator)
  File "main2.py", line 41, in two
    raise Exception("ha!")
Exception: ha!

Having the rest of the dependencies, un-"torndown" (?).

This, again in my opinion, is a less desired result. I think that the teardown logic should attempt to teardown all dependencies (by reverse order) while accumulating exceptions if they occur, and if any did, raise them at the end of the process as one aggregated exception.

Again the Connection / Transaction example can explain this desired behavior, if some error occurs during the teardown of the Transaction dependency, we would like the logic to still attempt to teardown the Connection object.

Would love your opinion on this,
Thanks in advance!

@proofit404
Copy link
Owner Author

@mastern2k3 thanks for the feedback! It's very informative! I'm appreciated your effort 👍

I'm totally agree with direction of your thoughts on this 👏

Even added one more thing to teardown logic #549

Problems with order and some parts of error handling could be easily addressed. I'm planning to do it during next week.

Behavior of collecting errors into some kind of group could take more time, and I still want to give myself a chance to think more about this problem before jumping into implementation of the first thing which came to my mind.

I would provide additional details in newly created tickets 🧑‍🏭

Have a good day 🌴 🍸

Cheers,
Artem.

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

Successfully merging a pull request may close this issue.

3 participants