BrighterCommand / BrighterCommand/Brighter

Scoped should mean per-chain for mappers and transforms, with opt-in adoption of a caller's scope

Open
#4,256 2 comments 0 reactions 1 assignee Claimed by @iancooper View on GitHub
.NET 2 - In Progress Bug V10.X
Dominant language
C#
Stars
2.5k
Forks
296
Avg merge
1d 11h
Merged PRs (30d)
21

Description

### Summary

Brighter has three scopes for objects it creates via supplied factories for handlers, transformers, and mappers.

- **Transient**: We create the object, use it, then release it.
- **Scoped**: We create the objects at the beginning of the request pipeline, and we tear them down at the end of the request pipeline. Where Brighter is used as a Command Dispatcher from an ASP.NET application, this scope is inherited from the HTTP Request; where Brighter is used as a Command Dispatcher from a Proactor or Reactor pump, the Proactor/Reactor should create a scope when it gets a message from the pump (before deserializing it via a transformer/mapper pipeline) and tears the scope down when the message terminates with ack/reject/requeue etc and we move to the next message on the pump. To be clear the lifetime is one message (to allow for per-message state). Fresh message, fresh scope.
- ** Singleton**: We create the objects once, and they last for the remainder of the program lifetime, usually at program startup.

Today, **scoped** does not work this way. Under `MapperLifetime.Scoped` (and the equivalent transform lifetime), the scope is bound to the **Dispatcher/process lifetime**, so state can be cached across messages and is only reclaimed at host shutdown.

### Current behaviour (as of #4254)

- `BuildDispatcher` (`Paramore.Brighter.ServiceActivator.Extensions.DependencyInjection/ServiceCollectionExtensions.cs`) is a **singleton** factory, so `ServiceProviderMapperFactory` / `ServiceProviderTransformerFactory[Async]` capture the **root** `IServiceProvider`.
- Under `MapperLifetime.Scoped`, `ServiceProviderLifetimeScope.GetOrCreateScoped` creates **one** `IServiceScope` off that root provider and caches every scoped mapper in it for the factory's entire lifetime. Fresh message → *same* scope.
- Nothing on the consumer pump (`Reactor`/`Proactor`/`Dispatcher`) opens a per-message DI scope: a grep for `CreateScope` / `IServiceScopeFactory` across `Paramore.Brighter.ServiceActivator` finds no call site. Handlers get per-resolution scopes via `ServiceProviderHandlerFactory`, but there is no unifying message-boundary scope.

### Why this matters

- A `Scoped` mapper/transform holds state (and any `IDisposable` it owns) from first use until process exit, rather than per message.
- The reviewer's suggestion on #4254 to dispose the consumer factories when the `Dispatcher` disposes only addresses **shutdown hygiene**; it does not make the scope per-message and would entrench "scope == Dispatcher lifetime" as the model.

### Proposed direction

> [!NOTE]
> **Superseded.** The direction below — one DI scope per dequeued message, shared by the mapper registry, the transform factories and the handler factory — was the starting point, but exploring it showed the scope unit should be the **chain** (one handler pipeline, or one transform pipeline), with the message as the *bound* rather than the scope itself. That model preserves [ADR 0039](https://github.com/BrighterCommand/Brighter/blob/master/docs/adr/0039-scoping-dependencies-inline-with-lifetime-scope.md), leaves the handler side already correct, and needs no change to the pump loop.
>
> **See the design narrative in the comment below** for the current direction, the evidence behind it, the constraints that shape it (notably: Microsoft's DI scopes do not nest), and the open questions.

~~Open a DI scope per dequeued message on the consumer pump and resolve the mapper registry, transform factories, and (ideally, aligned) handler factory from **that** scope, disposing it when the message completes (acked/rejected). This makes `Scoped` mean per-message on the consumer, matching the request-scope model, and removes cross-message state retention.~~

This remains an architectural change with its own test surface — deliberately **out of scope** for the memory-leak fix in #4254, which is confined to reclaiming the per-resolution `Transient` scopes that leaked in #4252.

### Related

- Follow-up from PR #4254 (fixes #4252), finding 4 of the latest review.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.