[Code review question] How to do teardown?

[sonthonaxrk]

Thanks for this project. I'm evaluating this as a way of managing dependencies in this large flask application I'm refactoring.

How would one manage cleanup of threadlocals, when you have to release things manually. In SQLAlchemy is this required, or anything that uses a threadpool.

I've made a working attempt (based off your example), but I'm not sure if it's the right direction.

class SQLAlchemyModule(Module):
    def __init__(self, app):
        self.app = app

    def configure(self, binder):
        db = self.configure_db()
        binder.bind(Session, to=self.create_session, scope=request_scope)

    def configure_db(self):
        connection_string = (
            'postgresql+psycopg2://{user}@{host}/{db}'
        ).format(
            user=self.app.config['PGSQL_USER'],
            host=self.app.config['PGSQL_HOST'],
            db=self.app.config['PGSQL_DB'],
        )

        self.engine = create_engine(
            connection_string,
            convert_unicode=True,
        )

        self.Session = scoped_session(sessionmaker(bind=self.engine))
        self.app.teardown_request(cleanup_session)

    def create_session(self) -> Session:
        return self.Session()

    def destroy_session(self, *args):
        self.Session.remove()

---

Is there anything I could do to make this class more idiomatic with Injector?

What I think would be really lovely, instead of binding to a function, you could bind to something with a yield statement.

So create_session would become

    @contextmanager
    def create_session(self) -> Session:
    try:
         yield self.Session()
    finally:
        self.Session.close()

[jstasiak]

Your ideal scenario (the one with yield statement) looks interesting and I think we should explore this, but for now the code you pasted above is the best one can do I believe (I've used something like that before, it's not elegant but it does the job, so...).

[sonthonaxrk]

@jstasiak

I made a stab at something that could do the job.

class ContextualProviderWrapper(CachedProviderWrapper):
    def __del__(self):
        for value in self._cache.values():
            if inspect.isgenerator(value):
                try:
                    next(value)
                except StopIteration:
                    pass

    def get(self, injector):
        key = id(injector)
        instance = self._old_provider.get(injector)
        if key not in self._cache:
            self._cache[key] = next(instance())

        return self._cache[key]

        
class ContextualRequestScope(RequestScope):
    def get(self, key, provider):
        try:
            return self._locals.scope[key]
        except KeyError:
            new_provider = ContextualProviderWrapper(provider)
            self._locals.scope[key] = new_provider
            return new_provider


contextual_request_scope = ScopeDecorator(ContextualRequestScope)

def setup_teardown():
     # set up
     yield
     # tear down

def configure(binder)
       binder.bind(
            bind_key,
            to=lambda: setup_teardown,
            scope=contextual_request_scope
        )

Works for me. But I'd love your review :)

[jstasiak]

Seems like a nice little hack :)

Two notes to self:

• I don't even remember what the CachedProviderWrapper thing is for and why I added it, I need to find out and either document or remove it
• The Scope and Binder classes need some type hints so that it's clear what's what

As for your solution – my main concerns here would be:

• Tying handling of generator-based providers to particular scope class
• Nondeterministic cleanup timing (__del__ can be called at arbitrary point in the future)
• I believe __del__ can be called effectively by any thread in your application (it'll be the one that triggers a garbage collection step), and that will be an issue as it won't operate on the correct thread-local storage.

While I believe a general-purpose scope-cleanup mechanism would be nice we're not there yet I'm afraid. I don't think cleaning things up manually here is too terrible all things considered.

[sonthonaxrk]

The cached CachedProviderWrapper is there to prevent multiple calls to app.injector.get() from calling the bound function multiple times.

I'll try and add type hints.

About your concerns

Tying handling of generator-based providers to particular scope class

So we could use inspect.isgenerator to decide weather to treat the provider as a generator. This would give the user the flexibility to use either a yield or return.

Nondeterministic cleanup timing (del can be called at arbitrary point in the future)

I think this is deterministically bound to the lifespan werkzurg locals, and is called when werkzurg releases them. However I could invoke a cleanup function manually. I agree it's a bit of a codesmell.

---

If I submit a PR with these changes would you be happy to merge it?

[jstasiak]

The note about type hints was for myself – I need to add them (the relevant ones) to injector first and to flask_injector afterwards, it's a bit of a spaghetti situation, don't worry about it.

I don't think __del__ will be called deterministically in all scenarios – GC is nondeterministic on PyPy and, if an object is a part of a reference cycle, on CPython as well. The request scope's cleanup method is called on request teardown though, so you use something like:

class CleaningUpRequestScope(RequestScope):
    def cleanup(self) -> None:
        # run cleanup generators here
        super().cleanup()
    def get(self, key, provider):
        # save cleanup generators somewhere here
        # ...

# ...

FlaskInjector(request_scope_class=CleaningUpRequestScope)

This will give you full determinism.

I'm unlikely to accept a PR with this – among other things because passing a callable returning a generator to Binder.bind is violating its interface (once I add type hints there it'll be an explicit error if tool like mypy is used) and only working thanks to a custom scope. And changing Binder.bind interface to allow this would mean an injector-wide solution would need to be provided, which I don't think is necessarily practical or desired anyway.

Also – cleaning things up seems so application-specific that I wouldn't necessarily want to involve a dependency injection library with it.