-
Notifications
You must be signed in to change notification settings - Fork 707
Fix swallowed exceptions in DistributedApplicationEventing #11900
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: davidfowl <[email protected]>
Co-authored-by: davidfowl <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11900Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11900" |
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 the issue where exceptions thrown in distributed application event handlers (like ResourceEndpointsAllocatedEvent) were being silently swallowed, making debugging extremely difficult. The solution introduces comprehensive exception handling that captures and logs all event handler failures while maintaining the non-blocking nature of the eventing system.
Key Changes
- Created
PublishEventExceptioninternal event to capture exception details when event handlers fail - Enhanced
DistributedApplicationEventing.PublishAsyncwith try-catch blocks for all dispatch behaviors - Added exception logging in
ApplicationOrchestratorthat logs to both resource-specific and general loggers
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Aspire.Hosting/Eventing/PublishEventException.cs |
New internal event class to capture exception details with event type and associated resource |
src/Aspire.Hosting/Eventing/DistributedApplicationEventing.cs |
Added comprehensive exception handling across all dispatch behaviors with recursive exception prevention |
src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs |
Added exception event subscription and logging to both resource-specific and general loggers |
tests/Aspire.Hosting.Tests/Eventing/DistributedApplicationBuilderEventingTests.cs |
Added comprehensive test coverage for exception handling across all dispatch behaviors |
| } | ||
|
|
||
| // Also log to a general logger using IServiceProvider | ||
| var logger = _serviceProvider.GetService<ILogger<ApplicationOrchestrator>>(); |
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.
@copilot Use GetRequiredService
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.
Done. Changed to use GetRequiredService in commit 7c3a8b1.
Co-authored-by: davidfowl <[email protected]>
|
@copilot rethrow the exception to the caller after publishing the event. |
Co-authored-by: davidfowl <[email protected]>
Done. Exceptions are now rethrown after publishing the PublishEventException event in commit 5e8f9a3. Updated tests to expect the exceptions to be thrown. |
|
I like the idea of an exception event, but I wonder whether a cheaper alternative might just be to simply grab the resource logger for resource scoped events and catch the exceptions log them. There are pros and cons - it could be quite noisy on the resource log view, but it would be right in your face and difficult to miss. |
| await Task.WhenAll(pendingSubscriptionCallbacks).ConfigureAwait(false); | ||
| }, default); | ||
| // Non-blocking concurrent - fire and forget | ||
| _ = Task.WhenAll(pendingSubscriptionCallbacks); |
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.
This may not be ideal. You don't necessarily get to choose who your fire and forget partners are. If there is an event that is going to hang around waiting for interaction (for example) it would stop you being notified that your event threw an exception.
The benefit of the event is that we can handle errors in publish mode as well. |
| await Task.WhenAll(pendingSubscriptionCallbacks).ConfigureAwait(false); | ||
| }, default); | ||
| // Non-blocking concurrent - fire and forget | ||
| _ = Task.WhenAll(pendingSubscriptionCallbacks); |
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.
@copilot this code is not ideal because it means that any reporting of event handlers that throw exceptions is delayed until all event handlers have completed.
Instead of doing this start a Task to publish exceptions which reads from a shared Channel. Inside the exception handler that you added write the exception into the channel.
The Task.WhenAll(...) can stay, but should also include the task for the exception publishing task.
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.
Done. Changed to use an unbounded Channel with a background task to process exceptions asynchronously in commit 0c4d1a2. This ensures exceptions are reported immediately without waiting for all event handlers to complete.
Co-authored-by: mitchdenny <[email protected]>
|
@copilot change |
…t public Co-authored-by: mitchdenny <[email protected]>
Done. Changed to public and renamed to |
Fixes #9769
Problem
Exceptions thrown in event handlers (such as
ResourceEndpointsAllocatedEvent) were being silently swallowed with no logging or error reporting. This made debugging extremely difficult since:For example, this code would fail silently:
Solution
This PR implements comprehensive exception handling in the
DistributedApplicationEventingsystem:1. Created
EventPublishExceptionEventPublic EventA public event that captures exception details when event handlers fail, including:
This event is public to allow external consumers to subscribe to exception notifications during event publishing.
2. Updated Event Publishing Logic with Channel-based Exception Reporting
Modified
DistributedApplicationEventing.PublishAsyncto catch exceptions in all dispatch behaviors:When an exception occurs, it's immediately queued to an unbounded
Channelfor asynchronous processing by a background task. This ensures instant exception reporting without waiting for all event handlers to complete. After queuing, exceptions are rethrown to the caller to ensure they're aware of the failure.3. Added Exception Logging in ApplicationOrchestrator
The
ApplicationOrchestratornow subscribes toEventPublishExceptionEventand logs errors to:This provides full visibility into event handler failures while still informing callers about exceptions.
4. Added Comprehensive Tests
Added tests verifying exception handling works correctly for:
Impact
Before: Event handler exceptions were completely invisible
After: All exceptions are immediately logged with full context AND rethrown to the caller
This change improves the developer experience by:
EventPublishExceptionEventthat allows external code to observe and react to event handler exceptions in both run and publish modesFixes #9769
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.