fix(subscriptions): keep factory-registered handlers separate - #577
Merged
Conversation
`AddEventHandler(Func<IServiceProvider, THandler>)` registered the handler as a keyed singleton under `(THandler, SubscriptionId)` and resolved it back by type. When several handlers were added to one subscription through this overload and `THandler` inferred to the same type for each call - which happens naturally when the factories are held in a collection typed `Func<IServiceProvider, IEventHandler>` - every registration after the first silently no-opped on the taken key, so the subscription ran N copies of the first handler and none of the others. Nothing threw and nothing was logged. Factory-created handlers are now memoised in the registration closure instead of the container, so each registration keeps its own handler. The same applies to the `AddCompositionEventHandler` overloads that build the inner handler from a factory. Since the container no longer holds them, the subscription now owns factory-created handlers and disposes them with the consume pipe, after the filters have drained the messages in flight. Handlers resolved from the container or supplied by the caller are left to their owners, and wrapping handlers are decorators, so they aren't disposed either. Adding the same handler type twice to one subscription used to dispatch every event to the same instance twice, just as silently. Both type-based overloads now claim the container slot up front and throw on the second claim. Closes #576 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
PR Summary by QodoFix factory handler registration collapse in subscriptions
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95be2a5a59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+55
to
+56
| THandler? handler = null; | ||
| AddHandlerResolve(sp => handler ??= Own(getHandler(sp))); |
There was a problem hiding this comment.
When getHandler returns an existing container-owned service—for example, sp => sp.GetRequiredService<SharedHandler>()—Own records that shared instance and EventSubscription.DisposeAsync now disposes it even though the container and other consumers may still use it, followed potentially by a second disposal during provider shutdown. This overload previously left the handler under container lifetime management, and the same regression exists in both composition overloads that call Own; ownership must not be inferred merely from using a provider-based factory.
Useful? React with 👍 / 👎.
Contributor
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Test Results 44 files + 21 44 suites +21 12m 33s ⏱️ - 1m 43s Results for commit f9b1629. ± Comparison against base commit 9722064. This pull request removes 26 and adds 29 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
A handler factory is free to return a handler it didn't create, most often `sp => sp.GetRequiredService<SharedHandler>()`, so inferring ownership from the use of a provider-based factory let the subscription dispose a container singleton other components still hold, and dispose it a second time at provider shutdown. Ownership is now stated by the caller through new `ownsHandler` and `ownsInnerHandler` overloads, and defaults to nobody owning the handler. They are overloads rather than optional parameters so assemblies compiled against the current signatures keep working. `ConsumePipe.DisposeAsync` guarded itself with a plain bool, which two callers can both pass, and which told the loser the teardown was finished while it was still running. `SubscriptionGateway` produces exactly that: `DisposeAsync` walks the subscriptions without removing them while `RemoveConnectionAsync` removes and stops them, so a disconnect racing the gateway shutdown disposes one pipe twice. The flag is now an interlocked exchange, and the callers that lose the race await the single teardown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alexeyzimarev
added a commit
to Eventuous/eventuous-docs
that referenced
this pull request
Aug 21, 2026
Follows the review on Eventuous/eventuous#577: a handler factory may return a handler owned elsewhere, so the subscription only disposes handlers the caller explicitly hands it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generic overload is the container-owned path, so a provider-based factory that only resolves an existing service is the odd case, not the norm, and defaulting to no ownership left the common case - a handler the factory builds - with nobody to dispose it. It also regressed against the behaviour before this branch. The old keyed registration meant the container captured whatever the factory returned and disposed it at shutdown, including a shared instance the factory merely resolved, which the container then disposed twice. Owning by default restores that disposal for every factory registration, and disposes a resolved shared handler once rather than twice. `ownsHandler` and `ownsInnerHandler` stay available to decline ownership when the factory returns a handler owned elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #576.
The bug
AddEventHandler(Func<IServiceProvider, THandler>)registered the handler as a keyed singleton under(THandler, SubscriptionId)and resolved it back by type. Add several handlers to one subscription through this overload withTHandlerinferring to the same type each time — which happens naturally when the factories live in a collection typedFunc<IServiceProvider, IEventHandler>— and every registration after the first silently no-ops on the taken key. The subscription then runs N copies of the first handler and none of the others, with nothing thrown and nothing logged.The two
AddCompositionEventHandleroverloads that build the inner handler from a factory had the identical collapse.The fix
Factory-created handlers are memoised in the registration closure instead of the container, so each registration keeps its own handler. This matches the instance overload, which never touched the container.
Handler disposal. With the container out of the picture, nothing would dispose a factory-created handler, so the subscription now owns them:
ConsumePipedisposes them when the subscription is disposed, after the filters have drained the messages in flight, in reverse creation order, preferringIAsyncDisposable. Ownership is narrow:AddEventHandler<T>()AddEventHandler(handler)AddEventHandler(sp => …)AddCompositionEventHandler(getWrappingHandler)AddCompositionEventHandler(getInnerHandler, …)ConsumePipe.DisposeAsyncalso became idempotent — it wasn't, andEventSubscriptioncompensated with anInterlocked.Exchange. Disposing a caller's handler twice is worse than disposing a filter twice.Breaking change
Adding the same handler type twice to one subscription used to dispatch every event to the same instance twice, just as silently. Both type-based overloads now claim the container slot up front and throw
ArgumentExceptionon the second claim. Use the factory or instance overload when a subscription genuinely needs two handlers of the same type.Factory-registered handlers are also no longer resolvable via
GetRequiredKeyedService<THandler>(subscriptionId). That was never documented, and the instance overload never had it.Tests
HandlerRegistrationTestsandHandlerDisposalTests, all watched red first:IEventHandlerran as[First, First, First]; now[First, Second, Third]— asserted on the resolved set by sending a message through the subscription's pipe, not on the registration listCompositionHandlerTests.ShouldResolveCompositionHandlerWithFactoryasserted the inner handler was resolvable from the container, which this removes. It now captures the handler the factory built and asserts its injected dependency — same intent, no container round-trip.Verified on net10.0:
Eventuous.Tests.Subscriptions130/130,Eventuous.Tests29/29,Eventuous.Tests.Application21/21.dotnet build Eventuous.slnxclean — 0 errors, and the 70 warnings match the pre-change baseline.Docs PR to follow in
Eventuous/eventuous-docs.🤖 Generated with Claude Code