Fix Double Publish with PublishAsync

[gillima]

Messages are published twice but the callback is never invoked when using PublishAsync.
Fixes #148

[niemyjski]

Can we simplify this into something like this? We should keep this simple and Publish Internal doesn't need to be async as it's a synchronous operation and can run inline (the callback can be awaited).

PublishInternal<TMessage>(message);
// invoke callback here?

[gillima]

You right! Since the async void can't be awaited and is asynchronously therefor. I personally don't like async void at all but in this case it might make sense to use it for readability and performance reasons.

[gkarabin]

Prior to the mistake in my PR, the original code was async via BeginInvoke (and still is for the not NETSTANDARD case). As I understand it, BeginInvoke queues to a Thread Pool. The only way to be synchronous is to “join” with the EndInvoke method.

Unless you want to change that aspect of the Publish method, I think something needs to occur to avoid callbacks blocking the caller thread.

https://devblogs.microsoft.com/dotnet/migrating-delegate-begininvoke-calls-for-net-core/

[gillima]

@niemyjski, @gkarabin. Thanks for the quick and good feedback. I've tried to simplify the method as much as possible while keeping the interface. With the current void method I don't think there's a simpler way than using Task.Run for doing the publishing asynchronously.

Having async Tasks with cancellation support for asynchronous methods would be great but the refactoring would exceed the scope of a #148 bugfix for sure.

[niemyjski]

Thanks for the link! I think this looks good with the current signature (fire and forget). Are you happy with me merging this pr?

[gillima]

Sure, I'm absolute happy if this makes it into the code. Thanks!

[gkarabin]

It looks good to me!

[niemyjski]

Thanks for the PR! I left some comments :)

[gillima]

Ups, just got reminded by the compiler that async void methods without await are running synchronously. Therefor I changed to a non-async void method that starts a not awaited task...

This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.

[gkarabin]

Ups, just got reminded by the compiler that async void methods without await are running synchronously. Therefor I changed to a non-async void method that starts a not awaited task...

This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.

This probably fixes a different behavior change, in that .net framework would not observe any exceptions when using BeginInvoke. EndInvoke, which wasn’t called, would be needed for that. Awaiting the Task would cause exceptions to be observed. This most recent change returns that behavior.

[gillima]

Awaiting the Task would cause exceptions to be observed

The exception would be "visible" inside the PublishAsync method but not for the calling method. The issue is that you can't await async void methods. Without making PublishAsync returning a Task I don't think there's a way to get the exception outside of PublishAsync

[gkarabin]

Right. I think that the behavior on your PR allows NETSTANDARD/NETCORE apps to match the behavior of .NET Framework prior to #132. #132 was a mess (with respect to the TinyMessenger change).

[niemyjski]

Merged! Thanks again for the pr! If I could get some help with #136 that would be a massive help for pushing a release with this.