-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Propagate exceptions from command handlers correctly #7218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Propagate exceptions from command handlers correctly #7218
Conversation
This is to make `CommandHandlerInvokerMiddleware` correctly 'await' the task before accessing its result, similar to how this is done in `RequestHandlerInvokerMiddleware`. Without this change, attempting to access result of a cancelled task would result in TargetInvocationException / AggregateException which breaks the task processing in `BackgroundCommandSenderHostedService` due to unhandled exception, thus breaking the background queue and particularly in bulk dispatch workflow causing remaining workflows to not get dispatched
|
@dotnet-policy-service agree company="D.E. Shaw & Co., L.P." |
sfmskywalker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes exception propagation in the Elsa.Mediator command pipeline so cancellations/faults from command handlers surface as the original exceptions (instead of TargetInvocationException/AggregateException), preventing background command processing from breaking.
Changes:
- Update
CommandHandlerInvokerMiddlewaretoawaitthe handler task before reading itsResult, aligning behavior with the request pipeline. - Unwrap
TargetInvocationExceptionin handler reflection invocations so callers observe the original exception. - Add new unit tests covering successful execution, cancellation, and failure behavior for commands.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/unit/Elsa.Mediator.UnitTests/Elsa.Mediator.UnitTests.csproj | Adds a new unit test project for Elsa.Mediator behavior. |
| test/unit/Elsa.Mediator.UnitTests/CommandCancellationBehaviorTests.cs | Adds tests verifying command cancellation and exception propagation behavior. |
| src/common/Elsa.Mediator/Middleware/Command/Components/CommandHandlerInvokerMiddleware.cs | Awaits the invoked task before accessing Result to avoid wrapped exceptions on cancellation/faults. |
| src/common/Elsa.Mediator/Extensions/HandlerExtensions.cs | Unwraps TargetInvocationException for notification/command handler reflection calls. |
test/unit/Elsa.Mediator.UnitTests/CommandCancellationBehaviorTests.cs
Outdated
Show resolved
Hide resolved
test/unit/Elsa.Mediator.UnitTests/CommandCancellationBehaviorTests.cs
Outdated
Show resolved
Hide resolved
test/unit/Elsa.Mediator.UnitTests/CommandCancellationBehaviorTests.cs
Outdated
Show resolved
Hide resolved
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@greptile open a new pull request to apply changes based on the comments in this thread |
|
I cannot create a new pull request on your behalf because:
To apply the review feedback, you have two options: Option 1: Update the existing PR (Recommended) 1. Fix using System.Runtime.ExceptionServices;
// At line 58 and 79, replace:
throw ex.InnerException;
// With:
ExceptionDispatchInfo.Capture(ex.InnerException).Throw();2. Fix
Option 2: Ask the PR author Would you like me to provide the complete corrected code for each file? |
|
@avin3sh It looks like Copilot can't implement its suggestions because this is a PR from your fork, so it would be great if you could review the suggestions yourself and resolve them (manually or using your own coding agents, of course). Thanks again! |
Use ExceptionDispatchInfo to prevent misleading stacktrace. As well use disposable fixture to actually dispose CommandSender,
…-handler-invoker-exception-handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
src/common/Elsa.Mediator/Middleware/Command/Components/CommandHandlerInvokerMiddleware.cs
Show resolved
Hide resolved
- Refactor duplicated unwrapping logic into shared InvokeAndUnwrap helper method - Simplify CommandHandlerInvokerMiddleware to be consistent with RequestHandlerInvokerMiddleware (remove redundant await) - Add Threshold property to test project for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
src/common/Elsa.Mediator/Middleware/Command/Components/CommandHandlerInvokerMiddleware.cs
Show resolved
Hide resolved
src/common/Elsa.Mediator/Middleware/Command/Components/CommandHandlerInvokerMiddleware.cs
Show resolved
Hide resolved
test/unit/Elsa.Mediator.UnitTests/CommandCancellationBehaviorTests.cs
Outdated
Show resolved
Hide resolved
- Add ConfigureAwait(false) to await statements in both CommandHandlerInvokerMiddleware and RequestHandlerInvokerMiddleware for consistency and to follow library code best practices - Increase test timeout from 50ms to 100ms for better CI reliability while still maintaining adequate margin (handler has 500ms delay)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
src/common/Elsa.Mediator/Middleware/Request/Components/RequestHandlerInvokerMiddleware.cs
Show resolved
Hide resolved
|
@sfmskywalker I have made some more changes as per the Agent's suggestions, I don't think the more recent suggestions are very helpful / are scope-creep. There was a conflict caused by #7226 where Let me know if you think further changes are necessary. |
|
Looks good to me - thanks for the hard work @avin3sh ! |
This is to make
CommandHandlerInvokerMiddlewarecorrectly 'await' the task before accessing its result, similar to how this is done inRequestHandlerInvokerMiddleware.Without this change, attempting to access result of a cancelled task would result in TargetInvocationException / AggregateException which breaks the task processing in
BackgroundCommandSenderHostedServicedue to unhandled exception, thus breaking the background queue and particularly in bulk dispatch workflow causing remaining workflows to not get dispatched.There are also Unit Tests included but they may be overkill for this scenario, can remove.