Add 'handles_exceptions' to register_factory

[lewicki]

Hi,
I got back to the problem I encountered in #112
My solution solved my local problem, but was simply wrong as generic improvement.
I got new idea. What about adding a new property handles_exceptions: bool = False to register_factory/RegisteredService?

That way both cases would be handled correctly. The cleanup would be executed by default, but if the factory handles the exception, it would be able to clean up itself.

I'm curious what you think of that solution. My patch (unconditional passing of exception to context managers) works for me, but I'm not happy of maintaining it. If you agree with the idea, I may try to prepare a proper PR.

def str_factory():
    print("String factory started")
    yield "instance"
    print("String factory ended")  # executed even if exception is raised

def int_factory():
    try:
        print("Int factory started")
        yield 123
    except:
        print("Factory managed to handle exception")  # executed because we registered the factory with handles_exception=True
        raise
    finally:
        print("Int factory ended")

reg.register_factory(str, str_factory)
reg.register_factory(int, int_factory, handles_exceptions=True)
with svcs.Container(reg) as con:
    my_str = con.get(str)
    my_int = con.get(int)
    raise Exception("Any exception in context of container")

Close method would be altered to something like:

    def close(self, exc_type, exc_val, exc_tb) -> None:
        """
        Run all registered *synchronous* cleanups.

        Async closes are *not* awaited and a warning is raised.

        Errors are logged at warning level, but otherwise ignored.

        Hint:
            The Container can be used again after this. Closing it is an
            idempotent way to reset it.
        """
        for name, cm in reversed(self._on_close):
            try:
                if isinstance(cm, AbstractAsyncContextManager):
                    warnings.warn(
                        f"Skipped async cleanup for {name!r}. "
                        "Use aclose() instead.",
                        # stacklevel doesn't matter here; it's coming from a
                        # framework.
                        stacklevel=1,
                    )
                    continue
                # --start--modification--
                rs = self._registered_service_for_cm(cm)  # didn't look into that part yet ;)
                if rs and rs.handles_exception:
                    cm.__exit__(exc_type, exc_val, exc_tb)
                else:
                    cm.__exit__(None, None, None)
                # --end--modification--
            except Exception:  # noqa: BLE001
                log.warning(
                    "Container clean up failed for %r.",
                    name,
                    exc_info=True,
                    extra={"svcs_service_name": name},
                )

        if self._lazy_local_registry is not None:
            self._lazy_local_registry.close()
        self._on_close.clear()
        self._instantiated.clear()

[lewicki]

Hi @hynek . I see you're back. What do you think of this change? I may be able to look into it soon.

[hynek]

heya, sorry for the delays – it's been… a lot. :)

let me re-iterate what you want, because there was a bit of hence and forth:

Your end-goal is to know whether the context block succeeded or not – correct?

If so, I'm OK with the feature since it basically fulfills the standard context manager protocol that we're currently violating (on purpose and for good reasons) but I'm not super happy with the name, because it's a bit too general for a rather specific use case.

How about something like: suppress_exit_context=True? 🤔 (i.e. inverted logic)

[lewicki]

Yes, fully agree. suppress_context_exit?

[hynek]

sure, why not!

[lewicki]

Created draft PR with initial change #129

[hynek]

this has been merged