Support context managers#71
Conversation
|
Hi!
I also used AI, to review ( SummaryThe PR is well-written and the opt-in design (manage_context=False default) is commendable. The sync path is clean and contained. However, there are significant concerns before merging. 🔴 Scope / Feature Creep The sync half (ExitStack in ActivationScope) is modest: ~80 LoC and self-contained. The async half is a different story and is what opens the door you intentionally kept
The PR author is right that those are separate — but this PR is the foot in the door. Once AsyncActivationScope and AsyncContainerProtocol are public surface, the 🔴 The httpx Example Is a Footgun The PR description lists httpx as a concrete use case: "The same pattern applies to httpx clients…" This is incorrect and will cause real performance harm to users who follow it. Why it's wrong:
So for the httpx case, manage_context=True is not just unhelpful — it actively destroys connection pooling. The httpx example should be removed from the PR description The PR description should only use the SQLAlchemy async session as the motivating example (where a new session per request is the correct pattern). 🟡 Bug: Services.aget Creates an Orphaned Scope async def aget(self, desired_type, scope=None, *, default=...):
if scope is None:
scope = self.create_async_scope() # created but never __aexit__'d
return await scope.aget(desired_type, default=default)If scope is None, a new AsyncActivationScope is created but its aexit is never awaited. Any managed context managers entered during resolution will never be exited — 🟡 Late Validation (Registration vs. Resolution) The PR validates at resolution time, not registration time: This means if you register NotACM with manage_context=True, the error only surfaces the first time the service is resolved (possibly deep in a request handler), not at 🟡 slots Inheritance in AsyncActivationScope AsyncActivationScope defines ✅ What's Good
And also, I argued this is a lot of work and maintenance overhead to achieve something as simple as, for example: # new - automatic entering and exiting of session
async def get_topic_by_id(self, site_id: int, topic_id: int) -> Topic | None:
result = await self.session.execute(
select(TopicEntity).filter(
TopicEntity.id == topic_id,
TopicEntity.site_id == site_id,
)
)
record = result.scalar_one_or_none()
return topic_entity_to_domain_type(record) if record else None instead of (currently requires entering the async session): async def get_topic_by_id(self, site_id: int, topic_id: int) -> Topic | None:
async with self.session as session:
result = await session.execute(
select(TopicEntity).filter(
TopicEntity.id == topic_id,
TopicEntity.site_id == site_id,
)
)
record = result.scalar_one_or_none()
return topic_entity_to_domain_type(record) if record else NoneTo which the AI commented: Yes, exactly. The entire PR — ~900 lines added, 5 new public symbols, a new async state machine — is motivated by saving the async with wrapper at the call site. And that wrapper is actually doing something meaningful: it makes the session lifecycle explicit and visible in the code. The tradeoff is poor:
The second version in your example (current) is arguably better code: it's obvious where the session begins and ends. The first version hides that the session has a lifecycle at The right answer for that use-case is almost certainly a factory dependency — inject async_sessionmaker (or a Callable[[], AsyncSession]) and let the handler/unit-of-work own the async with. That's zero new rodi infrastructure, zero magic, and the lifecycle stays in plain sight. I thank you for the time you put into this and interesting idea, but I prefer not merging this and keep handling of async context explicit in user's code for the reasons pointed out above by |
|
Hi! Thank you for the detailed review and for taking the time to read through it so thoroughly. You make excellent points. Regarding the Regarding the concerns about feature creep and singletons: I just wanted to clarify that I did actually consider them (as mentioned in the PR description). I deliberately deferred that work to keep this initial PR smaller and easier to review. My intention was to create a separate follow-up PR for singleton support if this initial implementation was merged. However, I completely understand your reasoning—even if done in a separate PR, adding this feature sets a precedent and forces the library to eventually adopt complex DI lifecycle management, which goes against the goal of keeping I also wanted to quickly touch on three of your technical notes:
Thanks again for the feedback. |
Add opt-in context manager management via
manage_contextflagSummary
Adds opt-in support for rodi to enter and exit services that implement the
context manager protocol (
__enter__/__exit__and/or__aenter__/__aexit__).Behavior is controlled by a new
manage_context: bool = Falseparameter on theregistration methods. The default is
False, so all existing behavior ispreserved exactly.
Motivation
The rodi documentation page on
context managers
states the core problem directly:
Today rodi resolves the ambiguity by doing nothing: classes that implement the
context manager protocol are instantiated but never entered or exited. Users
have to wrap every call site themselves with
with/async with, or wireper-request middleware that mirrors what a DI container should already be
doing.
This PR resolves the ambiguity by giving the developer an explicit declaration
of intent.
manage_context=Trueon registration tells rodi "yes, please enterthis on resolve and exit it when the owning scope ends." The default
(
manage_context=False) preserves today's behavior verbatim, so no existingapp is affected.
A concrete case where this matters is SQLAlchemy's async session:
The same pattern applies to httpx clients, file handles, locks, tracing
spans, and anything else that wants entry on borrow / exit on return.
API
Behavior matrix
provider.create_scope()TypeErrorprovider.create_async_scope()scope end.
scope end (LIFO).
InvalidContextManagerRegistration(rationale below).Why singleton is not supported (yet)
Singletons live for the lifetime of the
Servicesprovider, but rodi has noexplicit teardown hook for the provider/container today. Supporting
manage_context=Truefor singletons would require introducing one (e.g.Services.dispose()or makingContaineritself a context manager), which isits own design discussion (atexit semantics, shutdown ordering, error
handling).
To keep this PR focused and reviewable, singleton support is deferred to a
follow-up issue. The current behavior is to raise
InvalidContextManagerRegistrationat registration with a clear messagepointing the user to
SCOPED/TRANSIENTor to manual management.Implementation notes
ManagedScopedProviderandManagedTransientProviderwrap any existinginner provider (
TypeProvider,ArgsTypeProvider,FactoryTypeProvider).This keeps the existing 6 provider classes completely untouched, so users
who don't opt in pay zero overhead on the hot resolve path.
ActivationScopegained a lazy_exit_stack: ExitStack | None. Drainedbefore
dispose()in__exit__, in LIFO order. Standardswap-and-call idiom for single-shot consumption.
AsyncActivationScopeis a new subclass that adds a lazy_async_exit_stack: AsyncExitStack | None. Resolution stays synchronous —agetflips an_async_modeflag for the duration ofServices.get,causing
_enter_contextto defer instances into a_pending_aenterlist. Once sync resolution returns,
agetdrains the list usingenter_async_context(for async CMs) orenter_context(for sync CMs)on the same
AsyncExitStack. Syncgetcalls on the same async scopealso route into that unified stack via an overridden
_register_sync_context, so cross-protocol exit order across mixedget+agetcalls is strictly LIFO regardless of which entry pointadded the instance.
AsyncContainerProtocolis a separateProtocoldeclaringaresolve.Containerimplements bothContainerProtocolandAsyncContainerProtocol. The existingContainerProtocolis unchanged,so sync-only third-party DI containers remain compatible. Frameworks that
want async resolution opt into it structurally by typing against
AsyncContainerProtocol(or againstContainerdirectly).Container.aresolveis a thin wrapper aroundscope.agetthat validatesthe scope type up front, so misuse fails with a clear
TypeErrorinsteadof silently leaking managed CMs into an un-awaited throwaway scope.
TrackingActivationScope.__exit__now callssuper().__exit__(...)insteadof
self.dispose()directly, so the new exit-stack draining runs for userswho opt into nested-scope tracking. Pre-existing tests confirm dispose
semantics are unchanged.
_enter_contextrather than at registration. Class-level checks atregistration are an easy follow-up; deferred because the factory path can't
reliably introspect the type the factory will return.
Backward compatibility
manage_contextdefaults toFalseeverywhere. All existing call sites,fixtures, and tests work without changes. No public symbol was renamed or
removed. New public symbols added:
AsyncActivationScopeAsyncContainerProtocolContainer.aresolveInvalidContextManagerRegistrationManagedScopedProvider,ManagedTransientProvider(intended internal butexposed for parity with the other provider classes already in the public
module surface)
Services.create_async_scope,Services.agetFollow-up (not in this PR)
A separate issue/PR will discuss singleton +
manage_context=True. The shapeof that work — provider-level dispose, atexit semantics, container-as-context-
manager — is large enough that bundling it here would make this PR much harder
to review.