Deadlock in synchronous GetBytes(int) method

[MaxBarraclough]

Under certain configurations, GetBytes(int) will deadlock. I've just had this happen to my ASP.Net code.

I believe this is because it contains an anti-pattern that Stephen Cleary describes and explains in his Don't Block on Async Code blog post.

Specifically the line https://github.com/kmaragon/Konscious.Security.Cryptography/blob/14d35c9/Konscious.Security.Cryptography.Argon2/Argon2.cs#L31

A workaround is to use the Task class to execute on the thread-pool while the calling thread waits, avoiding the deadlock:

var task = System.Threading.Tasks.Task.Run(async () =>
{
    var a = new Konscious.Security.Cryptography.Argon2i(...);
    a.DegreeOfParallelism = ...;
    a.MemorySize = ...;
    a.Iterations = ...;
    a.Salt = ...;
    byte[] h = await a.GetBytesAsync(...);
    return h;
});
var hh = task.Result;

Or, slightly more simply:

var task = System.Threading.Tasks.Task.Run( () =>
{
    var a = new Konscious.Security.Cryptography.Argon2i(...);
    a.DegreeOfParallelism = ...;
    a.MemorySize = ...;
    a.Iterations = ...;
    a.Salt = ...;
    Task<byte[]> h = a.GetBytesAsync(...);
    return h;
});
var hh = task.Result;

Is there really any need for an async version of the method? It's CPU-bound anyway, after all.

[charliefoxtwo]

Chiming in to confirm I have the same issue, also in ASP.NET. Implementing @MaxBarraclough 's solution seems to work.

[tbandixen]

Chiming in to confirm that the issue in WPF exists too. Implementing @MaxBarraclough 's workaround works.

[kmaragon]

Actually the async version of the method was exactly for this use case. It was to give the caller the flexibility to avoid this bug because the synchronous version of GetBytes() did the same thing your code is doing here, which can cause deadlocks. The issue raised in the article is not in the Argon2 library. It's in your line that says:

var hh = task.Result;

I was also doing the same thing in the synchronous code. And that was causing the deadlock unavoidably.

With the async method, it's easy to get around with task.ConfigureAwait(false) before you call Result, as a workaround to the blog post cited by the OP. But an even better way to do it is to make the Controller method itself Async and bubble the async all the way up to that method. I haven't released a new version in a long time. But my plan was to make that the default for the synchronous method in the next release. But you would still have this same issue with the code sample above. Because it's redoing the exact thing my synchronous method was doing that was causing issues for people.

PS - Argon2's strength is actually that it's inherently parallel. That's one of the key points to how it functions. The parallelism itself affects the outcome of the hash. As such, it's not CPU bound. It's core bound. So if these get executed on the thread pool via Tasks, there MUST be a wait somewhere. The GetBytesAsync call gives you the ability to choose when that wait happens and where. Without an Async method, the GetBytes implementation internally needs to do that Await and prescribe in what way it does so. Which it does, in a way that causes deadlocks for the exact same reason that the code above does. If you let ASP.NET figure out the wait and synchronization, by making the Controller method async, in most cases that will render the best result.

[kmaragon]

Updated comments above

[MaxBarraclough]

Hi Keef. Thanks for the reply, but I think you misread me.

The issue raised in the article is not in the Argon2 library.

Yes it is. The deadlock was occurring when my purely synchronous ASP.Net code called Argon2's synchronous GetBytes method, which contains the bug.

Because it's redoing the exact thing my synchronous method was doing that was causing issues for people.

Not so. My workaround works for charliefoxtwo, tbandixen, and myself. It is not the equivalent of the synchronous GetBytes method, as it executes on the thread-pool but blocks the caller thread, following Stephen Toub's Offload to Another Thread pattern, documented in Should I expose synchronous wrappers for asynchronous methods?

I agree that a ConfigureAwait(false) call would be good practice in the first code-fragment I gave - oops! It works fine without it though, and I don't think it's strictly needed, looking at Toub's article.

Which it does, in a way that causes deadlocks for the exact same reason that the code above does.

Again, and at the risk of labouring the point: my workaround doesn't deadlock, only the synchronous GetBytes method does.

[kmaragon]

I see. I read the code sample above as repro steps. A more simple way to accomplish this is to instead say:

    var a = new Konscious.Security.Cryptography.Argon2i(...);
    a.DegreeOfParallelism = ...;
    a.MemorySize = ...;
    a.Iterations = ...;
    a.Salt = ...;
    var h = a.GetBytesAsync(...);
    h.ConfigureAwait(false);
    return h.result;

But again, just having an async method all the way up the call chain is a better approach.

And to your point, it is indeed problematic to expose a synchronous wrapper for an Aynch method. But that bumps up against the fact that the GetBytes method is from the base class (DeriveBytes)[https://msdn.microsoft.com/en-us/library/system.security.cryptography.derivebytes(v=vs.110).aspx]. Which doesn't seem to consider a cryptographic algorithm that's distinctly multi-threaded. Which is why I was going to make the ConfigureAwait(false) the default in the GetBytes method instead so users on systems with pinned update threads (IIS + WPF) don't deadlock. It's still doing the forbidden wrapping of an async method. But in a way that will produce fewer issues from IIS/WPF users. But by in large, it is a better strategy to use async controller methods.

[MaxBarraclough]

You're right of course that purely async code is ideal, but I don't see anything preventing us satisfying the synchronous DeriveBytes#GetBytes(Int32) method. The fact that the implementation uses multi-threading doesn't prevent us from providing a synchronous method.

My existing ASP.Net code was purely synchronous, and making my code asynchronous would have been a great deal of work for little payoff.

I'm pretty confident your code fragment there will still deadlock.

The Task#ConfigureAwait method doesn't modify its Task instance, instead it returns a ConfiguredTaskAwaitable instance, which is intended to be used with the await keyword. It's not intended to be used with the Result property, which is of course blocking, unlike await.

In your code-fragment, the ConfigureAwait call is dead, as the returned ConfiguredTaskAwaitable instance is discarded.

The correct solution is to do as Stephen Toub recommends, and have GetBytesAsync run on the thread-pool, and not on the caller thread. This has to be done explicitly, such as with the System.Threading.Tasks.Task.Run method, as Toub shows.

Perhaps the associated thread-marshaling could be avoided by using Toub's Avoid Unnecessary Marshaling approach instead, but that would presumably be a much deeper (and more error-prone) change.

[MaxBarraclough]

Bump. This bug is a real killer.

[tbandixen]

Anything new here?

[kmaragon]

The latest version does as Max described now.

[tbandixen]

Thanks for your update, just linking the source here: https://github.com/kmaragon/Konscious.Security.Cryptography/blob/c55fd848ba827fd8aa6b284ed174993fecbe1565/Konscious.Security.Cryptography.Argon2/Argon2.cs#L40