Eventstore: TaskCompletionSource.SetResult yields thread back to user land - and it can cause deadlocks

[charlessolar]

Very interestingly - Task.SetResult continues execution where the Task was waiting - immediately breaking away from the thread who called SetResult.
I've been tracking down a deadlock I occasionally see and found out about this weird undocumented behavior.

In the client api SetResult is used to deliver all results from operations in https://github.com/EventStore/EventStore/blob/08c2bdf7dcadd154cffa549d273e3a8e4673c5a1/src/EventStore.ClientAPI/ClientOperations/OperationBase.cs#L81

What this ends up doing is the ThreadPool thread currently processing operations from the operation queue is hijacked by Task to continue running my code from where I awaited the result of say for example AppendToStreamAsync
Basically your operations thread running all the crucial operation info back and forth between client and server is at the mercy of whatever I put after await AppendToStreamAsync

Which I guess explains my deadlock and why my clients often get heartbeat timeouts from the store despite working perfectly (my own code takes too long for the operation queue to process heartbeats)

Thankfully the fix is pretty simple - either you can wrap _source.SetResult in Task.Run OR every TaskCompletionSource can be constructed with TaskCreationOptions.RunContinuationsAsynchronously

Heres some additional info that I found researching the problem:

http://blog.stephencleary.com/2012/12/dont-block-in-asynchronous-code.html
http://stackoverflow.com/questions/19481964/calling-taskcompletionsource-setresult-in-a-non-blocking-manner#19497317

EDIT Seems there's a nice sized performance boost from this small change too - at least it seems that way to me in my testing

[gregoryyoung]

This basically tells me async should just never be used, I complained in
general about the move towards it at is buggy and hard to rationalize
about. You should SetResult in another Task.Run? That is ridiculous.

On Tue, Jan 17, 2017 at 3:50 AM, Charles Solar [e-mail]
wrote:

Very interestingly - Task.SetResult continues execution where the Task was
waiting - immediately breaking away from the thread who called SetResult.
I've been tracking down a deadlock I occasionally see and found out about
this weird undocumented behavior.

In the client api SetResult is used to delivery all results from
operations in https://github.com/EventStore/EventStore/blob/
08c2bdf7dcadd154cffa549d273e3a8e4673c5a1/src/EventStore.
ClientAPI/ClientOperations/OperationBase.cs#L81

What this ends up doing is the ThreadPool thread currently processing
operations from the operation queue is hijacked by Task to continue running
my code from where I awaited the result of say for example
AppendToStreamAsync
Basically your operations thread running all the crucial operation info
back and forth between client and server is at the mercy of whatever I put
after await AppendToStreamAsync

Which I guess explains my deadlock and why my clients often get heartbeat
timeouts from the store despite working perfectly (my own code takes too
long for the operation queue to process heartbeats)

Thankfully the fix is pretty simple - either you can wrap
_source.SetResult in Task.Run OR every TaskCompletionSource can be
constructed with TaskCreationOptions.RunContinuationsAsynchronously

Heres some additional info that I found researching the problem:

http://blog.stephencleary.com/2012/12/dont-block-in-asynchronous-code.html
http://stackoverflow.com/questions/19481964/calling-taskcompletionsource-
setresult-in-a-non-blocking-manner#19497317

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/EventStore/EventStore/issues/1179, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXRWvrCCh2fi0AIpjT6e2db1vRJAUZSks5rTDpugaJpZM4LlM9v
.

--
Studying for the Turing test

[charlessolar]

@gregoryyoung I would tend to agree - but I think the problem here is just the fact that ES is mixing async and sync operations. From what now know about async/await its best to either do it 100% or not at all.

For sure there is a solid performance boost to using async/await but there are definite problems when mixing .Wait and .Result and await

For instance if the whole api was Task based you would be using .ConfigureAwait(false) on every task await which avoids this whole issue with .SetResult

Edit Also Task.Run is hard to swallow but I've built the client api using TaskCreationOptions.RunContinuationsAsynchronously everywhere and that solved the issue too

[gregoryyoung]

My comment (to the slack channel) is my preference would be to rip it all out. It has not provided anything resembling performance increases over original design but has caused lots of instability and questions from people who don't know how to properly use async/await (eg they try in console apps then deadlock themselves)

[charlessolar]

I'd be up for performing an experiment - in researching this problem I think I know enough ES I could spike a task based ES.. at least for basic IO, append and read.

Though I'm not too familiar with the test client benchmarking. If you supply some hard-hitting test client commands I can come back to you with performance results on my hardware for release-v4.0.0 vs. a task based spike. It would be a moderately fun weekend project.

I just don't want to give up on async/await without offering a fair comparison, the benefits I got when NSB went 6.0 were not small.

[gregoryyoung]

What I meant is the OLD way was faster than it currently is. Though it used a thread per catchupsubscription as example. It was however also far more stable and understandable. If you note the backend does not really use async/await at all. This is not accidental.

[charlessolar]

I figured out a nice and simple way to demonstrate the issue - the code is here: https://github.com/volak/EventStoreProjectionIssue/blob/issue1179/EventStoreIssue/Program.cs

Notice at line 70 i'm waiting on a mutex right after AppendToStream - looks perfectly acceptable.. ACCEPT that wait is blocking the client api's operation queue due to this behavior causing a heartbeat timeout from the store.

I set my heartbeat timeout to 5 seconds for the testing.
And I tested with both the suggested solutions and didn't get a timeout

Realllllllly silly microsoft

Edit and waiting is just a silly example, calculating the first million prime numbers would do the same thing. Anything that is not another await to release the task again will interrupt the operation queue

[danielmarbach]

Executing the continuation of the TCS synchronously is a performance boost in 99% of the cases. The problem is ES mixture of sync and async like pointed out already

[rodolfograve]

Issues #861, #1051 and #1144 are all asking for support for async handlers and better usage of thread pool threads.

[charlessolar]

Closing as it seems this was fixed by other async improvements