Skip to content
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

Is there a RaiseAsync method for raising Func<..., Task> events? #1310

Closed
simonness opened this issue Dec 12, 2022 · 8 comments · Fixed by #1313
Closed

Is there a RaiseAsync method for raising Func<..., Task> events? #1310

simonness opened this issue Dec 12, 2022 · 8 comments · Fixed by #1313
Assignees
Milestone

Comments

@simonness
Copy link

simonness commented Dec 12, 2022

E.g. mock.Raise(x => x.ProcessMessageAsync += null, args); throws a System.Reflection.TargetParameterCountException : Parameter count mismatch. I notice Raise accepts an Action rather than Func. Is there a RaiseAsync method or similar?

public IUnderTest
{
    event Func<ProccessMessageEventArgs, Task> ProcessMessageAsync;
}
@stakx
Copy link
Contributor

stakx commented Dec 12, 2022

Your issue is essentially a duplicate of #1285. (You need to cast args to something other than EventArgs or a type derived from it; e.g. cast it to (object)args. See that other issue for a more detailed expalantion. That being said, this won't resolve your issue completely...)

Given that you're also enquiring about a RaiseAsync method, let's rephrase this issue as a feature request for RaiseAsync.

@stakx stakx changed the title How to call Raise when event handler is Func<EventArgs, Task> Is there a RaiseAsync method for raising Func<..., Task> events? Dec 12, 2022
@stakx
Copy link
Contributor

stakx commented Dec 12, 2022

Quoting from #977 (comment):

We could [...] add a mock.RaiseAsync method specifically for events whose handlers return a Task. Raise would then invoke each handler, gather its Task, and either await them in series or in parallel (using Task.WhenAll).

RaiseAsync hasn't been implemented yet. I'm also unsure whether subscribed event handlers (when there are more than one) should be raised in series or in parallel. Any thoughts?

@simonness
Copy link
Author

Thanks very much @stakx, makes sense, this feature would be greatly appreciated.

With regards to the in series vs parallel question, I'd be inclined to copy what the .net framework would do. That's not 100% clear to me in the case of events with a Task return type, but it would appear "in series" is the answer:

When an event has multiple subscribers, the event handlers are invoked synchronously when an event is raised.

https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/events/

@stakx
Copy link
Contributor

stakx commented Dec 13, 2022

copy what the .net framework would do. That's not 100% clear to me in the case of events with a Task return type, but it would appear "in series"

I think it would be a safe bet to execute them in series, for the reason you've mentioned. The downside being worse performance than if we executed them in parallel.

I've given this some thought too today. I think it's generally good practice that multiple event handlers should not be interdependent on one another. They should therefore not have to make any assumptions about the order in which they get invoked (even though the .NET runtime pretty much guarantees that they will execute in the order in which they were subscribed to the event).

So I think we could start by invoking all event handlers in parallel (which also happens to be easier to implement), but officially leave the order in which RaiseEvent invokes event handlers undocumented / unspecified. This way, we can "test-drive" parallel invocation and see if it actually causes any problems in practice. If not, that great because RaiseAsync will have ideal performance from the get-go. If, OTOH, we do get reports stating that parallel execution is problematic, we can still switch to serial invocation.

Makes sense?

Btw. I've created a draft implementation of RaiseAsync (see PR above) in case you want to give it a try.

@stakx stakx self-assigned this Dec 13, 2022
@simonness
Copy link
Author

simonness commented Dec 14, 2022

Thanks @stakx.

I found this blog post interesting and might be useful for making your decision - https://blog.stephencleary.com/2013/02/async-oop-5-events.html, perhaps eventually you could give the consumer the option of in series vs parallel.

It's also interesting that returning a Task from an event was / is debatable. To give some extra context I'm trying to work out the best way to write component tests that make use of Azure.Messaging.ServiceBus (the latest Azure Service Bus package), in particular the ServiceBusProcessor that has a public event Func<ProcessMessageEventArgs, Task> ProcessMessageAsync. It's already been pointed out that this library is harder to test.

@stakx
Copy link
Contributor

stakx commented Dec 14, 2022

Thanks for the link to that blog post. Quoting the second solution mentioned there:

If you take this approach, you could use Delegate.GetInvocationList to invoke each handler individually and Task.WhenAll to detect when they have all completed.

It's reassuring to see Stephen Cleary (who is very knowledgeable about all things async) suggest the exact same thing that I've already implemented as a draft. So we're probably on the right track!

@stakx
Copy link
Contributor

stakx commented Dec 14, 2022

It's also interesting that returning a Task from an event was / is debatable.

Given that C# and VB.NET's syntax for raising an event does not give you any way of retrieving the event handlers' individual return values, it's understandable — what would be the point of returning values when they're almost certain to be ignored / get dropped? Noone wants to tinker around with GetInvocationList every time the event is raised, that's for exceptional / advanced use cases.

So much for the historical reason. Then the TPL was introduced in C# 4 and async/await even later; that's why Stephen Cleary writes:

make your event handler delegate type return Task instead of void. This is not as evil as it first appears; it’s true that event handlers are supposed to return void, but Task is the async equivalent of void, so the event handler delegate type still logically returns void, even though it doesn’t literally return void.

The non-generic Task / ValueTask types are probably about the only non-void return types that make sense for event delegate types, for the reason mentioned.

@stakx stakx added this to the 4.19.0 milestone Dec 30, 2022
@stakx
Copy link
Contributor

stakx commented Dec 30, 2022

Let's do this! I'm leaving the exakt invocation order of subscribed event handlers (in series vs. in parallel) unspecified for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants