Convert ValueTask to a Task before blocking

[pakrym]

Fixes #10904

[pakrym]

@AlexanderSher ping

[AlexanderSher]

EnsureCompleted shouldn't be called on non-completed ValueTask at all, that's what we check in VerifyTaskCompleted. We're using EnsureCompleted in the policies that may be executed in any TaskScheduler/SyncContext combination and sync waiting on incomplete task can easily hang user code.

If debug-only exception isn't enough and throwing InvalidOperationException for release build isn't an option, we need to discuss it further. Is there any real repro behind #10904?

[pakrym]

I understand that we should never be calling EnsureCompleted() on non-completed tasks but if the case we accidentally do I'd like to make a best effort and keep customers apps working. Even if sync-over-async is suboptimal and might cause other issues in 90% of the cases it will work just fine.

What kind of repro are you looking for? That ValueTasks don't allow GetAwaiter().GetResult() on incomplete tasks or that we do sync-over-async somewhere?

[AlexanderSher]

Then we need additional check to ensure that we're trying to wait incomplete task synchronously only on threadpool, something like:

(SynchronizationContext.Current == null || SynchronizationContext.Current.GetType() == typeof(SynchronizationContext)) && TaskScheduler.Current == TaskScheduler.Default

I'd prefer to throw exception rather than introduce possible deadlock.

[pakrym]

Do you think it's required considering we are using ConfigureAwait(false) everywhere?

[AlexanderSher]

In a sync code path ConfigureAwait(false) doesn't change anything even if used. In async code path, user can add custom policy that interacts with UI (progress bar or abort/retry message) that may bring execution of the following policy into sync context.

[pakrym]

This situation would only happen when sync code accidentally runs an async code path.

I'm not saying there is no way to deadlock at all but considering we use ConfigureAwait(false) in all our async code the probability that one of our calls would need to be scheduled on sync context we also don't often call back into customers code this solution would work in 90% on the unlikely cases we miss with analyzers.

[AlexanderSher]

we also don't often call back into customers code

I would say it is an expected scenario when user adds UI interaction as per retry policy. How else can I update UI to provide notification that request has failed and is going to retry?

[pakrym]

What are you proposing?

Throwing an exception in the case where customers code would've had a high chance of working?

There is also no workaround customer can do if we throw an exception, they can always workaround blocking sync-over async and get their code working.

[AlexanderSher]

Yes, I think user must be aware that we have a bug in our code that can potentially hang an application. If we throw an exception, user will know where the bug is and apply workaround. If we hang, it would be much harder to figure out why.

The workaround is simple: explicitly call failing API method using Task.Run so that sync context isn't captured.

[pakrym]

Hm, good point, let me try the changes.