feat(eventbus): 增加EventBus服务#2878
Conversation
Reviewer's Guide引入一个与生命周期感知(lifecycle-aware)的静态 EventBusService 发布与处理器调用的时序图sequenceDiagram
actor Publisher
participant EventBusService
participant HandlerObject as IEventHandler_MyEvent
Publisher->>EventBusService: PublishAsync(channel, MyEvent)
EventBusService->>EventBusService: _CallChannelAsync(channel, MyEvent)
EventBusService->>EventBusService: _CallEventHandlerAsync(MyEvent, dataHandlers)
EventBusService-->>HandlerObject: HandleEventAsync(MyEvent)
HandlerObject-->>EventBusService: Task
EventBusService-->>Publisher: Task
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 来:
Getting HelpOriginal review guide in EnglishReviewer's GuideIntroduce a lifecycle-aware, static EventBusService that supports typed, channel-based publish/subscribe using a common EventDataBase payload type and disposable subscriptions, along with interfaces for async event handlers and unit tests validating delegate-based and IEventHandler-based subscriptions and unsubscription behavior. Sequence diagram for EventBusService publish and handler invocationsequenceDiagram
actor Publisher
participant EventBusService
participant HandlerObject as IEventHandler_MyEvent
Publisher->>EventBusService: PublishAsync(channel, MyEvent)
EventBusService->>EventBusService: _CallChannelAsync(channel, MyEvent)
EventBusService->>EventBusService: _CallEventHandlerAsync(MyEvent, dataHandlers)
EventBusService-->>HandlerObject: HandleEventAsync(MyEvent)
HandlerObject-->>EventBusService: Task
EventBusService-->>Publisher: Task
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - 我发现了两个问题,并给出了一些整体性的反馈:
- 两个 Subscribe 重载都重复了相同的通道 / 处理程序注册与反注册逻辑;可以考虑把公共代码提取到一个共享的辅助方法中,以减少重复并保持行为一致。
- 在 _StopAsync 中,对基于 IEventHandler 的订阅去释放其 Owner 可能会让人意外,因为处理程序实例的所有权在调用方;可以考虑不要在这里释放外部处理程序,或者通过文档 / 重命名让这一生命周期责任表达得更清晰。
- 如果 IResponsibleEventHandler 确实打算废弃,请把注释替换为 [Obsolete] 特性(或直接移除该接口),这样调用方可以在编译期得到明确的信号,而不是依赖代码注释。
面向 AI 代理的提示
请根据此次代码审查中的评论进行修改:
## 整体评论
- 两个 Subscribe<TEventData> 重载都重复了相同的通道 / 处理程序注册与反注册逻辑;可以考虑把公共代码提取到一个共享的辅助方法中,以减少重复并保持行为一致。
- 在 _StopAsync 中,对基于 IEventHandler 的订阅去释放其 Owner 可能会让人意外,因为处理程序实例的所有权在调用方;可以考虑不要在这里释放外部处理程序,或者通过文档 / 重命名让这一生命周期责任表达得更清晰。
- 如果 IResponsibleEventHandler<TResponse> 确实打算废弃,请把注释替换为 [Obsolete] 特性(或直接移除该接口),这样调用方可以在编译期得到明确的信号,而不是依赖代码注释。
## 单独评论
### 评论 1
<location path="PCL.Core/App/EventBus/EventBusService.cs" line_range="30-32" />
<code_context>
+ {
+ foreach (var entry in handlersByType.Values)
+ {
+ if (entry.Owner is IDisposable d)
+ {
+ try { d.Dispose(); } catch { /* ignore */ }
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** 如果同一个处理程序实例被多次订阅,在这里释放 `Owner` 可能会被执行多次。
由于 `Owner` 是按订阅存储的,同一个实例可以被注册多次,从而在 `_StopAsync` 中被重复释放。如果这里的某些 `IDisposable` 实现不是幂等的,就可能引入一些隐蔽的 bug。可以在关闭过程中跟踪已经释放过的 owner(例如使用带有合适相等性比较的 `HashSet<object>`),或者把释放的责任下沉到订阅 / owner 本身,这样 `_StopAsync` 就不需要直接调用 `Dispose`。
</issue_to_address>
### 评论 2
<location path="PCL.Core/App/EventBus/IResponsibleEventHandler.cs" line_range="5-6" />
<code_context>
+
+namespace PCL.Core.App.EventBus;
+
+// I think this is too hard to implement. So this is Obsolete.
+public interface IResponsibleEventHandler<TResponse>
+{
+ /// <summary>
</code_context>
<issue_to_address>
**suggestion:** 该接口在注释中被标为过时(obsolete),但没有使用特性标记,而且其参数 `EventDataBase` 类型过于宽泛。
既然打算废弃这个接口,请考虑要么直接删除它,要么用 `[Obsolete]` 标记,避免产生新的用法。如果要保留,建议为事件数据增加一个类型参数(例如 `IResponsibleEventHandler<TEventData, TResponse> where TEventData : EventDataBase`),这样事件数据和响应类型可以保持关联,并与 `IEventHandler<TEventData>` 以及事件总线 API 的其余部分保持一致。
</issue_to_address>请帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈持续改进评审质量。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- Both Subscribe overloads duplicate the same channel/handler registration and unregistration logic; consider extracting the common code into a shared helper to reduce duplication and keep behavior consistent.
- In _StopAsync, disposing the Owner for IEventHandler-based subscriptions may be surprising since the handler instance is owned by the caller; consider either not disposing external handlers here or documenting/renaming to make this lifecycle responsibility explicit.
- If IResponsibleEventHandler is intended to be deprecated, replace the comment with an [Obsolete] attribute (or remove the interface) so callers get a clear compile-time signal instead of relying on a code comment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both Subscribe<TEventData> overloads duplicate the same channel/handler registration and unregistration logic; consider extracting the common code into a shared helper to reduce duplication and keep behavior consistent.
- In _StopAsync, disposing the Owner for IEventHandler-based subscriptions may be surprising since the handler instance is owned by the caller; consider either not disposing external handlers here or documenting/renaming to make this lifecycle responsibility explicit.
- If IResponsibleEventHandler<TResponse> is intended to be deprecated, replace the comment with an [Obsolete] attribute (or remove the interface) so callers get a clear compile-time signal instead of relying on a code comment.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/EventBus/EventBusService.cs" line_range="30-32" />
<code_context>
+ {
+ foreach (var entry in handlersByType.Values)
+ {
+ if (entry.Owner is IDisposable d)
+ {
+ try { d.Dispose(); } catch { /* ignore */ }
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Disposing `Owner` here can run multiple times if the same handler instance is subscribed more than once.
Since `Owner` is stored per subscription, the same instance can be registered multiple times and thus disposed repeatedly in `_StopAsync`. If any `IDisposable` here is not idempotent, this can introduce subtle bugs. Either track already-disposed owners (e.g., via a `HashSet<object>` with appropriate equality) during shutdown, or move disposal responsibility into the subscription/owner so `_StopAsync` doesn’t call `Dispose` directly.
</issue_to_address>
### Comment 2
<location path="PCL.Core/App/EventBus/IResponsibleEventHandler.cs" line_range="5-6" />
<code_context>
+
+namespace PCL.Core.App.EventBus;
+
+// I think this is too hard to implement. So this is Obsolete.
+public interface IResponsibleEventHandler<TResponse>
+{
+ /// <summary>
</code_context>
<issue_to_address>
**suggestion:** The interface is commented as obsolete but not marked with an attribute and has a loosely-typed `EventDataBase` parameter.
Since this is intended to be obsolete, please either remove the interface or mark it with `[Obsolete]` to avoid new usages. If you keep it, consider adding a type parameter for the event data (e.g. `IResponsibleEventHandler<TEventData, TResponse> where TEventData : EventDataBase`) so the event data and response types stay associated and consistent with `IEventHandler<TEventData>` and the rest of the event bus API.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体性的反馈:
- 如果
IResponsibleEventHandler<TResponse>本身是有意不打算被使用的,建议不要只加注释说明,而是实际标记上[Obsolete](或者直接移除),这样使用方在编译期就能得到提示。 - 在
_CallEventHandlerAsync中,当发布的事件没有匹配的处理程序时会记录错误日志并抛出异常;如果这种情况在正常使用中是可预期的,你可能需要将其视为 no-op(或者至少不要抛异常),以避免在没有任何订阅者时出现无意义的失败。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- If `IResponsibleEventHandler<TResponse>` is intentionally not meant for use, consider actually marking it with `[Obsolete]` (or removing it) rather than only leaving a comment, so consumers get compile-time guidance.
- In `_CallEventHandlerAsync`, publishing an event with no matching handlers logs an error and throws; if this situation is expected in normal usage, you may want to treat it as a no-op (or at least not throw) to avoid spurious failures when nothing is subscribed.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/EventBus/IResponsibleEventHandler.cs" line_range="1-6" />
<code_context>
+
+namespace PCL.Core.App.EventBus;
+
+// I think this is too hard to implement. So this is Obsolete.
+public interface IResponsibleEventHandler<TResponse>
+{
+ /// <summary>
</code_context>
<issue_to_address>
**suggestion:** Consider marking this interface as [Obsolete] instead of using a comment.
The code comment marks this interface as obsolete, but the compiler can’t enforce that. Prefer adding an `[Obsolete("Use X instead")]` attribute (and planning for eventual removal) so callers are warned and new usages are discouraged.
```suggestion
using System;
using System.Threading.Tasks;
namespace PCL.Core.App.EventBus;
[Obsolete("IResponsibleEventHandler is obsolete and will be removed in a future version.")]
public interface IResponsibleEventHandler<TResponse>
```
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- If
IResponsibleEventHandler<TResponse>is intentionally not meant for use, consider actually marking it with[Obsolete](or removing it) rather than only leaving a comment, so consumers get compile-time guidance. - In
_CallEventHandlerAsync, publishing an event with no matching handlers logs an error and throws; if this situation is expected in normal usage, you may want to treat it as a no-op (or at least not throw) to avoid spurious failures when nothing is subscribed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If `IResponsibleEventHandler<TResponse>` is intentionally not meant for use, consider actually marking it with `[Obsolete]` (or removing it) rather than only leaving a comment, so consumers get compile-time guidance.
- In `_CallEventHandlerAsync`, publishing an event with no matching handlers logs an error and throws; if this situation is expected in normal usage, you may want to treat it as a no-op (or at least not throw) to avoid spurious failures when nothing is subscribed.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/EventBus/IResponsibleEventHandler.cs" line_range="1-6" />
<code_context>
+
+namespace PCL.Core.App.EventBus;
+
+// I think this is too hard to implement. So this is Obsolete.
+public interface IResponsibleEventHandler<TResponse>
+{
+ /// <summary>
</code_context>
<issue_to_address>
**suggestion:** Consider marking this interface as [Obsolete] instead of using a comment.
The code comment marks this interface as obsolete, but the compiler can’t enforce that. Prefer adding an `[Obsolete("Use X instead")]` attribute (and planning for eventual removal) so callers are warned and new usages are discouraged.
```suggestion
using System;
using System.Threading.Tasks;
namespace PCL.Core.App.EventBus;
[Obsolete("IResponsibleEventHandler is obsolete and will be removed in a future version.")]
public interface IResponsibleEventHandler<TResponse>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
b6553bd to
423d962
Compare
|
all done, wait for review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并留下了一些整体性的反馈:
IResponsibleEventHandler<TResponse>上的注释写着它已过时,但是该接口本身并没有标记为过时;建议要么添加一个带有清晰说明信息的[Obsolete]特性,要么在你准备好支持这种模式之前移除这条注释。- 如果通道不是通过
AddChannel创建的,两个Subscribe重载都会抛出异常并记录错误日志;如果这不是设计上严格要求的行为,建议要么在首次使用时自动创建通道,要么返回一个更清晰、且不记录日志的失败路径,以避免产生噪声日志和难以诊断的运行时错误。 - 当在
_CallEventHandlerAsync中找不到任何处理程序时,该方法总是记录错误;如果对某些事件来说“没有订阅者”是正常情况,建议将该日志级别降级为 debug/trace,或者做成可配置,以避免在典型流程中产生过多的错误日志。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The comment on `IResponsibleEventHandler<TResponse>` says it is obsolete but the interface itself is not marked as such; consider adding an `[Obsolete]` attribute with a clear message or removing it until you are ready to support this pattern.
- Both `Subscribe` overloads throw and log an error if the channel was not created via `AddChannel`; if this is not strictly required by design, consider either auto-creating channels on first use or returning a clearer, non-logged failure path to avoid noisy logs and hard-to-diagnose runtime errors.
- When no handlers are found in `_CallEventHandlerAsync`, the method always logs an error; if "no subscriber" is a normal situation for some events, consider downgrading this to a debug/trace-level log or making it configurable to avoid excessive error logging in typical flows.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/EventBus/EventBusService.cs" line_range="70" />
<code_context>
+ }
+
+ var dataType = typeof(TEventData);
+ var handlers = dataHandler.GetOrAdd(dataType, _ => new ConcurrentDictionary<Guid, (Func<EventDataBase, Task>, WeakReference<object>?, bool)>());
+
+ var ownerRef = new WeakReference<object>(handler);
</code_context>
<issue_to_address>
**suggestion:** The `OwnsOwner` flag in the handler tuple is never read and can be removed or wired into the disposal logic.
The handler dictionary stores a `(Func<EventDataBase, Task>, WeakReference<object>?, bool OwnsOwner)` tuple, but this flag is never read; disposal is based only on `disposeOwnerOnUnsubscribe` in the closure. Please either remove `OwnsOwner` from the tuple and its assignments, or use it in the disposal logic so the stored state matches the actual behavior.
Suggested implementation:
```csharp
var dataType = typeof(TEventData);
var handlers = dataHandler.GetOrAdd(dataType, _ => new ConcurrentDictionary<Guid, (Func<EventDataBase, Task>, WeakReference<object>?)>());
```
```csharp
var id = Guid.NewGuid();
handlers.TryAdd(id, (Wrapper, ownerRef));
```
There are likely other declarations/usages of this handler dictionary in `EventBusService.cs` (e.g., where the dictionary field is declared or iterated). Update all such occurrences to use the 2-tuple `(Func<EventDataBase, Task>, WeakReference<object>?)` instead of the 3-tuple with the `bool` flag, and remove any tuple deconstruction or access that refers to the `OwnsOwner` element.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The comment on
IResponsibleEventHandler<TResponse>says it is obsolete but the interface itself is not marked as such; consider adding an[Obsolete]attribute with a clear message or removing it until you are ready to support this pattern. - Both
Subscribeoverloads throw and log an error if the channel was not created viaAddChannel; if this is not strictly required by design, consider either auto-creating channels on first use or returning a clearer, non-logged failure path to avoid noisy logs and hard-to-diagnose runtime errors. - When no handlers are found in
_CallEventHandlerAsync, the method always logs an error; if "no subscriber" is a normal situation for some events, consider downgrading this to a debug/trace-level log or making it configurable to avoid excessive error logging in typical flows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The comment on `IResponsibleEventHandler<TResponse>` says it is obsolete but the interface itself is not marked as such; consider adding an `[Obsolete]` attribute with a clear message or removing it until you are ready to support this pattern.
- Both `Subscribe` overloads throw and log an error if the channel was not created via `AddChannel`; if this is not strictly required by design, consider either auto-creating channels on first use or returning a clearer, non-logged failure path to avoid noisy logs and hard-to-diagnose runtime errors.
- When no handlers are found in `_CallEventHandlerAsync`, the method always logs an error; if "no subscriber" is a normal situation for some events, consider downgrading this to a debug/trace-level log or making it configurable to avoid excessive error logging in typical flows.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/EventBus/EventBusService.cs" line_range="70" />
<code_context>
+ }
+
+ var dataType = typeof(TEventData);
+ var handlers = dataHandler.GetOrAdd(dataType, _ => new ConcurrentDictionary<Guid, (Func<EventDataBase, Task>, WeakReference<object>?, bool)>());
+
+ var ownerRef = new WeakReference<object>(handler);
</code_context>
<issue_to_address>
**suggestion:** The `OwnsOwner` flag in the handler tuple is never read and can be removed or wired into the disposal logic.
The handler dictionary stores a `(Func<EventDataBase, Task>, WeakReference<object>?, bool OwnsOwner)` tuple, but this flag is never read; disposal is based only on `disposeOwnerOnUnsubscribe` in the closure. Please either remove `OwnsOwner` from the tuple and its assignments, or use it in the disposal logic so the stored state matches the actual behavior.
Suggested implementation:
```csharp
var dataType = typeof(TEventData);
var handlers = dataHandler.GetOrAdd(dataType, _ => new ConcurrentDictionary<Guid, (Func<EventDataBase, Task>, WeakReference<object>?)>());
```
```csharp
var id = Guid.NewGuid();
handlers.TryAdd(id, (Wrapper, ownerRef));
```
There are likely other declarations/usages of this handler dictionary in `EventBusService.cs` (e.g., where the dictionary field is declared or iterated). Update all such occurrences to use the 2-tuple `(Func<EventDataBase, Task>, WeakReference<object>?)` instead of the 3-tuple with the `bool` flag, and remove any tuple deconstruction or access that refers to the `OwnsOwner` element.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
7875fa2 to
e1c098d
Compare
这个PR新增了EventBus服务,用来实现Core和UI等组件间的通讯。所有测试已通过(除
Unsubscribe_Prevents_Handler_Call因为LifeCycle的限制无法初始化Context会触发Exception,但这是预期行为)使用方法可以直接看单侧中的内容
Summary by Sourcery
引入一个由生命周期管理的 EventBus 服务,用于在应用组件之间进行类型安全、基于通道的通信。
新特性:
EventBusService,用于在命名通道上发布和订阅强类型事件,并支持感知生命周期的关闭以及自动处理程序清理。EventDataBase,作为事件负载的通用基础记录类型。IEventHandler<TEventData>,用于可释放的异步事件处理程序,并引入实验性接口IResponsibleEventHandler<TResponse>,用于基于响应的处理。测试:
IEventHandler的订阅,并确保在取消订阅后处理程序不会被调用。Original summary in English
Summary by Sourcery
Introduce a lifecycle-managed EventBus service for typed, channel-based communication between application components.
New Features:
Tests:
新功能:
EventBusService,用于在命名通道上发布强类型事件,并通过与生命周期关联的清理机制管理订阅。EventDataBase作为事件负载的通用基础记录类型。IEventHandler<TEventData>和实验性的IResponsibleEventHandler<TResponse>接口,用于异步事件处理。测试:
IEventHandler的订阅,以及取消订阅后处理程序不再被调用的场景。Original summary in English
Summary by Sourcery
引入一个由生命周期管理的 EventBus 服务,用于在应用组件之间进行类型安全、基于通道的通信。
新特性:
EventBusService,用于在命名通道上发布和订阅强类型事件,并支持感知生命周期的关闭以及自动处理程序清理。EventDataBase,作为事件负载的通用基础记录类型。IEventHandler<TEventData>,用于可释放的异步事件处理程序,并引入实验性接口IResponsibleEventHandler<TResponse>,用于基于响应的处理。测试:
IEventHandler的订阅,并确保在取消订阅后处理程序不会被调用。Original summary in English
Summary by Sourcery
Introduce a lifecycle-managed EventBus service for typed, channel-based communication between application components.
New Features:
Tests:
新功能:
EventBusService,用于在命名通道上发布和订阅类型化事件,并支持随生命周期自动清理。EventDataBase作为事件负载的通用基础记录类型。IEventHandler接口,用于可释放(disposable)、异步的事件处理程序,以及实验性的基于响应的处理程序接口IResponsibleEventHandler。测试:
IEventHandler的订阅,并确保取消订阅后不会再触发处理程序。Original summary in English
Summary by Sourcery
引入一个由生命周期管理的 EventBus 服务,用于在应用组件之间进行类型安全、基于通道的通信。
新特性:
EventBusService,用于在命名通道上发布和订阅强类型事件,并支持感知生命周期的关闭以及自动处理程序清理。EventDataBase,作为事件负载的通用基础记录类型。IEventHandler<TEventData>,用于可释放的异步事件处理程序,并引入实验性接口IResponsibleEventHandler<TResponse>,用于基于响应的处理。测试:
IEventHandler的订阅,并确保在取消订阅后处理程序不会被调用。Original summary in English
Summary by Sourcery
Introduce a lifecycle-managed EventBus service for typed, channel-based communication between application components.
New Features:
Tests:
新功能:
EventBusService,用于在命名通道上发布强类型事件,并通过与生命周期关联的清理机制管理订阅。EventDataBase作为事件负载的通用基础记录类型。IEventHandler<TEventData>和实验性的IResponsibleEventHandler<TResponse>接口,用于异步事件处理。测试:
IEventHandler的订阅,以及取消订阅后处理程序不再被调用的场景。Original summary in English
Summary by Sourcery
引入一个由生命周期管理的 EventBus 服务,用于在应用组件之间进行类型安全、基于通道的通信。
新特性:
EventBusService,用于在命名通道上发布和订阅强类型事件,并支持感知生命周期的关闭以及自动处理程序清理。EventDataBase,作为事件负载的通用基础记录类型。IEventHandler<TEventData>,用于可释放的异步事件处理程序,并引入实验性接口IResponsibleEventHandler<TResponse>,用于基于响应的处理。测试:
IEventHandler的订阅,并确保在取消订阅后处理程序不会被调用。Original summary in English
Summary by Sourcery
Introduce a lifecycle-managed EventBus service for typed, channel-based communication between application components.
New Features:
Tests: