Avoid calling Task.Result and Task.Run

[nathan-alden-sr]

It is well known that one should never call Task.Result, Task.Wait(), or Task.WaitAll() in normal code due to the possibility of deadlocks. I see a couple of places where you are calling the Task.Result property. You should rewrite those places like this: someTask.ConfigureAwait(false).GetAwaiter().GetResult(). This is the idiomatic way to run a task when inside a method that is not asynchronous.

There is also no need to call Task.Run unless you intend to either await the call or to fire-and-forget. If you need the result and you're inside a synchronous method, you should write the code as I've done above.

[Insomniak47]

@nathan-alden-sr question: not sure exactly how GetAwaiter.GetResult prevents deadlocks. My understanding of the only differences were that .Result throws an AggregateException if the task throws and GetResult() will throw the actual exception. For my benefit do you have any docs that explain this one a bit better?

[Dean-McMillan]

This has caught us out in our Azure function app in the GetBytes() method - deadlocks killed the processing. Is there a suggested way to avoid this? Currently looking at this as a way to avoid it via a Task.Run:

        var t = Task.Run(() => argon2.GetBytes(16));
        t.Wait();
        return t.Result;

OR... as most suggest, make sure that there is no sync/async code, that is making the Method that calls the library thus:

public static Task<byte[]> HashPassword(string password, byte[] salt)

[kmaragon]

The intention was to have people use the Async contracts. After several years in the wild, it's clear that that intention is either impractical for most porjects or is just being ignored. I've got some time to work on this for a few weeks. So expect a 2.0 release that just gets rid of any ability to do this async but will resolve these issues.

[artdedeco]

It caught my attention seeing async methods in a hashing lib. Please correct me if I missed something, but looking at the code it seems the "async" methods are actually just using Tasks for doing parallel CPU work. These are not what Tasks are meant for and can interfere with legitimate task scheduling.

I think we should remove remove the current (fake) async API, take that implementation and change it to use the TPL constructs for doing parallel CPU work (eg. Parallel.For) and make that the sync API (which is the only API the lib should expose, since it doesn't seem to be doing any IO).

[kmaragon]

https://github.com/dotnet/runtime/blob/3aa1ec5bb1f50f0a1bed9cfcac8734f742bcf24b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs#L572

Parallel.For uses tasks. It's all using the task thread pool. Using tasks is not fake parallelism, but it yields the parallelism to the scheduler.

It's a valid philosophical argument but there's not a right answer. Parallel.For uses tasks though elides an extra layer of task source creation when using the default scheduler. If you don't use the thread pool and spin up your own threads, then you get true parallelism (as far as the os can guarantee it via thread preemption) that can't be "pre-empted" by the default scheduler tasks in the managed runtime. So you also potentially screw the rest of the program by instead forcing everything into context switching hell under high load (see thread preemption).

So what you call fake parallelism is only fake in so far as it participates with the rest of the ecosystem which means it isn't guaranteed to be truly parallel (at least as far as the os scheduler can guarantee such) if the user has other work scheduled. But if there's nothing else happening and n < the number of threads in the default scheduler, it's truly parallel.

But for that same reason, Task.Result is a problem. Because now you have a scheduler thread that is blocked waiting for other scheduler threads to finish their work and if you do that enough times very quickly, none of the threads can finish any work because they are all waiting on each other to finish work. It was a crappy sacrifice made to conform to the Hash interfaces in .net, which were entirely non async, in the unusual cases that they were strictly required. I didn't expect every developer out there to reach for that instead of the async version, even when the async version was a perfectly valid option for the use case.

I don't have time to work on this right now. I also want to get arm intrinsics completed first in a branch that's been half done for like 1.5 years+. But I do want to replumb the back end to just use a private threadpool in the non async case so that developers that were too lazy/uninformed/whatever to use async stop saying I don't know what I'm doing because I gave them the rope to stupidly hang themselves. Then instead of deadlocking under their bad choices, the performance will just fall off a context switching hell cliff, which is certainly a much better option. The only one that is probably acceptable if I want to keep conforming to the interfaces.

[artdedeco]

Thanks for the insightful answer.

I didn't call it fake parallelism, rather I said "fake async". My point is mainly around how this is using async/await for something it was not designed for. As for how big or small the consequences of doing that, there are lots of nuances, as you're obviously aware. In the end, I'm just not seeing the benefit of bringing in the async/await plumbing and messing with API consumers and the default taskscheduler expectations.

I only looked superficially at the code and it seemed like it would be trivial to change it from async/await to using only the TPL. And, a priori, I think that would solve the sync issue as a bonus. But again, I didn't think too deeply about it yet, so pardon me if I'm missing something obvious.