-
Notifications
You must be signed in to change notification settings - Fork 640
Add TrySubscribeOnce API in IDistributedApplicationEventing #9461
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
@copilot try again, it looks like you may have had a transient error. |
Co-authored-by: mitchdenny <513398+mitchdenny@users.noreply.github.com>
I've implemented the TrySubscribeOnce API in the DistributedApplicationEventing interface and class as requested. The implementation adds support for both resource and non-resource variants with appropriate key-based idempotency checks. I also added tests to verify the functionality works correctly. See commit eb4c4e8. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot add default implementations on the interfaces which throw |
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
Add idempotent subscribe-once support for distributed application events via new TrySubscribeOnce
API overloads and verify their behavior through unit tests.
- Introduce four new
TrySubscribeOnce
overloads inIDistributedApplicationEventing
for non-resource and resource events, with and without custom keys. - Implement subscription deduplication in
DistributedApplicationEventing
using internal dictionaries keyed by event type, resource, and user-provided key. - Add unit tests in
DistributedApplicationBuilderEventingTests.cs
to confirm that only the first subscription attempt succeeds and callbacks fire as expected.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Aspire.Hosting/Eventing/IDistributedApplicationEventing.cs | Added TrySubscribeOnce method signatures with [NotNullWhen(true)] out parameters and overloads for resources |
src/Aspire.Hosting/Eventing/DistributedApplicationEventing.cs | Implemented each TrySubscribeOnce overload, managing subscription dictionaries to enforce idempotency |
tests/Aspire.Hosting.Tests/Eventing/DistributedApplicationBuilderEventingTests.cs | Added tests to assert that TrySubscribeOnce only succeeds once per key (and fires callbacks correctly) |
Comments suppressed due to low confidence (2)
src/Aspire.Hosting/Eventing/DistributedApplicationEventing.cs:129
- The Unsubscribe method removes the subscription from the event lists but does not remove it from _subscriptionKeyLookup or _resourceSubscriptionKeyLookup, preventing future TrySubscribeOnce calls with the same key from succeeding after an unsubscribe. Consider removing these entries when unsubscribing.
public void Unsubscribe(DistributedApplicationEventSubscription subscription)
tests/Aspire.Hosting.Tests/Eventing/DistributedApplicationBuilderEventingTests.cs:423
- There are no tests covering the behavior of resubscribing after calling Unsubscribe. Consider adding test(s) to verify that TrySubscribeOnce returns true again once a previous subscription is unsubscribed.
}
|
||
if (_eventSubscriptionListLookup.TryGetValue(typeof(T), out var subscriptions)) | ||
{ | ||
subscriptions.Add(subscription); |
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.
Adding to the List without synchronization can cause race conditions under concurrent subscriptions. Consider using a thread-safe collection or locking around list modifications.
Copilot uses AI. Check for mistakes.
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 can you look at this.
…stributedApplicationEventing interface Co-authored-by: mitchdenny <513398+mitchdenny@users.noreply.github.com>
I've added default implementations that throw Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
_innerBuilder.Services.AddSingleton<DashboardInjectionService>(); | ||
Eventing.Subscribe<BeforeStartEvent>((e, ct) => | ||
{ | ||
var injectionService = e.Services.GetRequiredService<DashboardInjectionService>(); | ||
injectionService.InjectDashboard(e.Model); | ||
return Task.CompletedTask; | ||
}); | ||
|
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.
@davidfowl looking at this change I'm not entirely certain we should get rid of IDistributedApplicationLifecycleHook
completely. There are instances where that approach works really well (such as these global things like injecting the dashboard).
I think where TrySubscribeOnce
comes in handy is more for hosting integrations where you may have multiple of a resource.
The Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Thanks for assigning this issue to me. I'm starting to work on it and will keep this PR's description up to date as I form a plan and make progress.
Original issue description:
Fixes #9042.
💡 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.