Eventstore: ClientAPI: thread pool exhaustion / Thread.Sleep in public async methods

[nappy]

We had an issue where we started 10000 async tasks using the client API (e.g. ReadStreamEventsForwardAsync) using the default TaskScheduler.

It went very badly, due to the while loop in EnqueueOperation:

private void EnqueueOperation(IClientOperation operation)
{
    while (_handler.TotalOperationCount >= _settings.MaxQueueSize)
    {
        Thread.Sleep(1);
    }
    _handler.EnqueueMessage(new StartOperationMessage(operation, _settings.MaxRetries, _settings.OperationTimeout));
}

As soon as the MaxQueueSize is reached, the EventStore Client starts to put thread pool threads on hold by synchronously waiting for the queue to accept new input.

At some point this leads to a situation where all threads are blocked, and as a result the TotalOperationCount cannot decrease anymore (because the client is using the default TaskScheduler internally too) and we end up in a situation where the entire thread pool is exhausted/dead.

In general if I search the code base for Thread.Sleep I get a lot of results which worries me for an async library. If you expose an async API I do not expect such things. Basically you should only call Thread.Sleep on threads you own, especially if its possible that they never complete.

Mixing blocking and non-blocking patterns in such fashion is very dangerous...

[pgermishuys]

@nappy thanks for detailing the issue. We will investigate.
Did you have a valid use case for starting that many tasks or were you testing something else like limits/ranges?

[nappy]

@pgermishuys At this point it was just a test case in an early stage of a project, but 10000 is our target connection count per node, so thats where the number came from. Its obviously still a synthetic test case, because in reality it would not happen that you queue up 10000 operations at the exact same moment.
But still I think it should be solid enough to handle that kind of load gracefully.

[gregoryyoung]

The max size of the queue is also configurable and you were running with a default. eg MaxQueueSize, use a larger one. There are only two choices here, back off or throw an exception.

[nappy]

@gregoryyoung Sure we already increased the MaxQueueSize as a result of this.
But I think the way the client deals with this (using Thread.Sleep) is not a good idea, and thats why I created this issue: It should either throw an exception or wait asynchronously. Increasing the MaxQueueSize feels more like a workaround, since we almost never want the Thread.Sleep call to be hit.

[gregoryyoung]

"Increasing the MaxQueueSize feels more like a workaround"

No its a setting. The whole point of it is to push back the threads overloading the queue.

As I said there are only two choices here. Wait or throw. You prefer throw for your use case. Most prefer wait. Maybe we should add a setting to throw.

[nappy]

In reality you are using the Thread Pool as a queue when it goes beyond this setting instead of limiting it, degrading performance/concurrency as a result, until it hits another limit later on at which point you are in a dead lock.

Also from a more general perspective: You should not run long running operations on the thread pool.
Waiting synchronously on other operations to complete makes this a long running task...

[haf]

I agree with @nappy here, that if his quote:

At some point this leads to a situation where all threads are blocked, and as a result the TotalOperationCount cannot decrease anymore (because the client is using the default TaskScheduler internally too) and we end up in a situation where the entire thread pool is exhausted/dead.

...is correct, then the it's not a backoff protocol but instead makes it possible to deadlock. Deadlock <> backoff. He's just stating that your variant is never decreasing and that you have a liveness issue.

[rodolfograve]

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

[gregoryyoung]

There was a reason why the original implementation used a thread and not
the threadpool. The later implementation was pushed by the community, I
disagreed with it.
https://github.com/EventStore/EventStore/commits/release-v4.0.2/src/EventStore.ClientAPI/EventStoreCatchUpSubscription.cs

On Tue, Dec 27, 2016 at 4:31 AM, Henrik Feldt [e-mail]
wrote:

I agree with @nappy https://github.com/nappy here, that if his quote:

At some point this leads to a situation where all threads are blocked, and
as a result the TotalOperationCount cannot decrease anymore (because the
client is using the default TaskScheduler internally too) and we end up in
a situation where the entire thread pool is exhausted/dead.

...is correct, then the it's not a backoff protocol but instead makes it
possible to deadlock. Deadlock <> backoff. He's just stating that your
variant is never decreasing and that you have a liveness issue.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/EventStore/EventStore/issues/1051#issuecomment-269320031,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXRWvRdLR1w73rJI1tPcP9tdgafeMgSks5rMQUqgaJpZM4J_AG0
.

--
Studying for the Turing test

[rodolfograve]

@gregoryyoung but instead of killing async, why not push forward and just provide a good implementation of it, where good means it plays nicely with other async calls by not blocking any thread pool threads?

I see enough push around from the community, and there's even a fork (https://github.com/volak/EventStore) that claims to solve at least one of the problems.

For use cases where the number of subscriptions is low, as my team's, using dedicated, non-thread pool threads, is an acceptable approach. However, as @nappy said on issue #1051, some people also want to be able to have thousands of subscriptions, and that means thread-per-subscription is not suitable.

In any case, I do think that unfortunately the current implementation is buggy. In our applications, having a few simultaneous SignalR requests (enough to make all thread pool threads busy), causes a disconnection due to a skipped heartbeat, even if there is no interaction with GetEventStore. We have been using GetEventStore for almost 2 years now and we only starting noticing this issue as the number of clients connected to SignalR increased.

Completely unexpected behavior, and it has took us a couple of weeks to find out the details, just to learn that there is no appropriate way to fix it other than changing the internals of the Client API, like @volak did in his fork.

We would be extremely grateful if the team decided to take this on, one way or another, ideally by implementing async correctly rather than dropping support for it.

The alternatives for us are to use a modified version of GetEventStore, or to drop it completely, but I don't think there's anything out there that can replace ES.

[nappy]

As I said mixing asynchronous patterns and synchronous patterns is the core issue here.
If the developers are more confident with a synchronous solution, then go for it, create a Thread for each subscription. The code I have seen indicates that its not written by people who fully understand the async concepts. For example the amount of "ContinueWith" constructs I have seen are also kind of alarming. Not that its bad per se, but its terrible to read and work with. The await keyword is there for a reason.
In theory rewriting synchronous code to async should pretty easy though, because async/await allows you to write your async code just like synchronous code. Its not bad to use the thread pool either, you only have to get rid of those blocking calls.

[gregoryyoung]

We do accept pull requests.

On Tue, May 16, 2017 at 5:13 AM, Julian [e-mail] wrote:

As I said mixing asynchronous patters and synchronous patterns is the core
issue here.
If the developers are more confident with a synchronous solution, then go
for it, create a Thread for each subscription. The code I have seen
indicates that its not written by people who fully understand the async
concepts. For example the amount of "ContinueWith" constructs I have seen
are also kind of alarming. Not that its bad per se, but its terrible to
read and work with. The await keyword is there for a reason.
In theory rewriting synchronous code to async should pretty easy though,
because async/await allows you to write your async code just like
synchronous code. Its not bad to use the thread pool either, you only have
to get rid of those blocking calls.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/EventStore/EventStore/issues/1051#issuecomment-301762977,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXRWu05q7w8h2FpyNVGR0OY6TU8WGw7ks5r6ZLxgaJpZM4J_AG0
.

--
Studying for the Turing test

[rodolfograve]

@gregoryyoung I'm happy to provide a PR for this, but I wouldn't want to waste my time. Would the team be happy to accept a PR to improve the async implementation, rather than removing it?

[gregoryyoung]

yes.

On Tue, May 16, 2017 at 1:52 PM, Rodolfo Grave [e-mail]
wrote:

@gregoryyoung https://github.com/gregoryyoung I'm happy to provide a PR
for this, but I wouldn't want to waste my time. Would the team be happy to
accept a PR to improve the async implementation, rather than removing it?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/EventStore/EventStore/issues/1051#issuecomment-301772305,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXRWvyz7AChjPmfzz-H1ry7x-G7i9DIks5r6Zv2gaJpZM4J_AG0
.

--
Studying for the Turing test

[rodolfograve]

Challenge accepted :)

[pgermishuys]

Looking forward to it @rodolfograve!

[rodolfograve]

Hi.

I have now created PR #1310 to try to tackle this. I think there is a lot more that could be done but in the spirit of keeping the PR as small as possible this only tries to add support for Func OnEventAppereared.

More details in the PR.

[jen20]

This looks like it has been resolved with #1567, so we'll close this issue out. If anyone feels that it is _not_ addressed, please feel free to re-open.