Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Scheduling & Latency

###Scheduling & Latency OneOne single handler could block all other handlers by performing a long-running calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this - especially since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

###Thread-Safety

Thread-Safety

While WeakReferenceList is thread-safe when adding and removing items, enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safeguards against registration during event notification.

###Error-Handling

Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

###Micro-optimisations

Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let the waiters handle the status, which is built in using async/await.

###Ambiguously-defined handlers

Ambiguously-defined handlers

Handlers that implement both interfaces are not able to define which of the interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

###Weak Reference Pattern

Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft:

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

###Scheduling & Latency One single handler could block all other handlers by performing a long-running calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this - especially since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

###Thread-Safety

While WeakReferenceList is thread-safe when adding and removing items, enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safeguards against registration during event notification.

###Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

###Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let the waiters handle the status, which is built in using async/await.

###Ambiguously-defined handlers

Handlers that implement both interfaces are not able to define which of the interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

###Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft:

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

Scheduling & Latency

One single handler could block all other handlers by performing a long-running calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this - especially since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

Thread-Safety

While WeakReferenceList is thread-safe when adding and removing items, enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safeguards against registration during event notification.

Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let the waiters handle the status, which is built in using async/await.

Ambiguously-defined handlers

Handlers that implement both interfaces are not able to define which of the interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft:

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

Spelling and grammar
Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325

Scheduling & Latency

One ###Scheduling & Latency One single handler could block all other handlers by performing a longrunninglong-running calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this. Specially, - especially since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

Thread-Safety ###Thread-Safety

While WeakReferenceList is thread-safe when adding, and removing items. Enumerating, enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safe-guardssafeguards against registration during event notification.

Error-Handling ###Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

Micro-optimisations ###Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let awaitersthe waiters handle the status, which is built in using asyncasync/awaitawait.

Ambigiously defined handlers ###Ambiguously-defined handlers

Handlers that implement both interfaces are not able to define which of boththe interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

Weak Reference Pattern ###Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft :

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

Scheduling & Latency

One single handler could block all other handlers by performing a longrunning calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this. Specially, since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

Thread-Safety

While WeakReferenceList is thread-safe when adding, removing items. Enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safe-guards against registration during event notification.

Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let awaiters handle the status, which is built in using async/await.

Ambigiously defined handlers

Handlers that implement both interfaces are not able to define which of both interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

###Scheduling & Latency One single handler could block all other handlers by performing a long-running calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this - especially since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

###Thread-Safety

While WeakReferenceList is thread-safe when adding and removing items, enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safeguards against registration during event notification.

###Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

###Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let the waiters handle the status, which is built in using async/await.

###Ambiguously-defined handlers

Handlers that implement both interfaces are not able to define which of the interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

###Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft :

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

added 459 characters in body
Source Link
dfhwze
  • 14.1k
  • 3
  • 40
  • 101

Scheduling & Latency

One single handler could block all other handlers by performing a longrunning calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this. Specially, since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

Thread-Safety

While WeakReferenceList is thread-safe when adding, removing items. Enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safe-guards against registration during event notification.

Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let awaiters handle the status, which is built in using async/await.

Ambigiously defined handlers

Handlers that implement both interfaces are not able to define which of both interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

Scheduling & Latency

One single handler could block all other handlers by performing a longrunning calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this. Specially, since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

Thread-Safety

While WeakReferenceList is thread-safe when adding, removing items. Enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safe-guards against registration during event notification.

Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

Ambigiously defined handlers

Handlers that implement both interfaces are not able to define which of both interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

Scheduling & Latency

One single handler could block all other handlers by performing a longrunning calculation:

public void Handle(SavingChangesEvent message)
{
 Thread.Sleep(10000); // long running code ..
}

While this is the exact same behavior of the default Event Pattern in C#, you might expect an aggregator to be able to work around this. Specially, since the method is called PublishAsync. It's not the best design decision to have this method perform both synchronous IHandle<T> and asynchronous IHandleAsync<T> event handler notifications.

In addition, if you have a single listener on EventT1 and 1000 listeners on EventT2, you'd always loop all those listeners (even twice in your code) to find the listeners OfType<RequestedType>(). Using one container list for all listeners is something I would try to avoid. A dictionary by Type and its listeners is a better approach.

Thread-Safety

While WeakReferenceList is thread-safe when adding, removing items. Enumerating the items while adding, removing items concurrently is not. You'd have to implement a custom threading mechanism that safe-guards against registration during event notification.

Error-Handling

Any error in a synchronous-aware handler exits notifications early. Since the synchronous notifications take priority over the asynchronous ones, these won't even be notified. Perhaps implementing an UnobservedExceptionHandler could help you make a robust design.

Micro-optimisations

Filtering out Where(t => t.Status != TaskStatus.RanToCompletion) and returning Task.CompletedTask when handler.Any() is false, is not going to gain you much. In fact, in between checking the status and returning the completed task, the status may have already been changed. I would disregard the status and always return return Task.WhenAll(handlers). Let awaiters handle the status, which is built in using async/await.

Ambigiously defined handlers

Handlers that implement both interfaces are not able to define which of both interfaces they want to see handled. By default, both their handlers will be notified. Is this as designed or should more fine-grained registration be allowed?

Weak Reference Pattern

I am not sure using weak references is the best design decision here. According to Microsoft

Avoid using weak references as an automatic solution to memory management problems. Instead, develop an effective caching policy for handling your application's objects.

And I would agree. Instead I would use registration and deregistration methods.

Source Link
dfhwze
  • 14.1k
  • 3
  • 40
  • 101
Loading
lang-cs

AltStyle によって変換されたページ (->オリジナル) /