Fix usage of GetAwaiter()

[martincostello]

JustSayingFluently uses Task.GetAwaiter().GetResult() in two places.

We should see if we can do something to remove these, or failing that, provide a "safer" way to make the asynchronous calls synchronous (TaskCompletionSource?).

[slang25]

TaskCompletionSource can't really help us here, the only fix I can see will be to make the initialization with something like a BuildAsync in place of Build. Or to move the initialization into the start listening / first publish, which I think would be less ideal.

[martincostello]

Fair enough. Maybe this is something we can tackle as part of whatever comes out of #431.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[maurofranchi]

@martincostello is this still open? May I look at it?

[martincostello]

@maurofranchi I believe this one is pending us finishing the test migration to the new fluent API. Once we've done that, we can start to delete the old API, which is where these two usages live.

[martincostello]

@maurofranchi See #471.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

Closed automatically due to inactivity.

[martincostello]

This isn't completed. There's still 5 uses of GetAwaiter().GetResult() in the codebase.

[shaynevanasperen]

https://github.com/shaynevanasperen/Code/blob/master/src/Code/Threading/SafeAsync.cs

Available on NuGet here: https://www.nuget.org/packages/Code.Threading.SafeAsync/

[slang25]

It's worth restating the problem with GetAwaiter().GetResult():

1. It could lead to a deadlock if there is a current SynchronizationContext that requires exclusivity of delegate execution.
2. If the thread the blocking happens on is a threadpool thread, this could lead to theadpool exhaustion.

For point 1, we don't expect typical usage of the library to have a SynchronizationContext present at the point the fluent-api is called, for example if it was a full framework ASP.NET app, we would expect the setup to be done at startup, and not lazily during a web request (which would deadlock).
That said, I've seen people do this (and had to fix it).

For 2, generally it's good practice to not ignore this issue, however it's should only cause issues where you are repeatedly blocking threadpool threads, and here it's a one-off activity maybe a few of these calls will happen when the app first starts up.

So it could be worth mitigating 1 with the technique @shaynevanasperen is referring to, and ultimately adding an async initialization method.

We had removed all other blocking calls where it mattered most, but had 2 (at least) attempts to get rid of the initialization blocking calls, but we never quite got it right, it required a breaking change and it would always lead to us saying "Well if we are going to make such a big breaking change, we may as well fix all of the things around the broken fluent api", then the change would be too big to get in. With @martincostello BusBuilder work, it feels like this would be a good time to re-address this.

[martincostello]

For v7 we should make it async all the way down. Then if there's any async gymnastics needed at the root caller into our API surface it can be done once there.

The laziness should make that easy to do as everything would just be deferred to publish/subscribe time, and if necessary we can make a "do it now" method, and then the integrator is responsible for doing a sync async call, not us.

[slang25]

Agreed. So just to make sure we are on the same page, we could have an InitializeAsync method, that could be eagerly awaited, but if not it would be called on the first call toStart (it makes sense for it to be StartListeningAsync) or PublishAsync.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[gkinsman]

This is now completed as of PR #749