PreserveAsyncOrder=false with async message handler can cause thread pool exhaustion & extremely bad performance

[niemyjski]

I'm running the following code on .net core 2.0. When PreserveAsyncOrder = false is false and if the message handler containing any async code (Cache invalidation etc, change notifications that send messages over web sockets....) can cause major performance issues as well as thread pool exhaustion as the following code shows... When you change it to true, there are no extra threads and the same code runs in a fraction of the time..

Other notes..

We saw this occurring in multiple applications that use Foundatio message bus (https://github.com/FoundatioFx/Foundatio.Redis). We previously had async void which is really really bad (dotnet/roslyn#13897). Once we switched it a single GetAwaiter().GetResult() we noticed that the thread pool would become exhausted and our asp.net core app would fall over with a 502.5/503 errors. We are working on moving all this async code into a TaskQueue but that brings in it's own problems (back pressure, unbounded queues etc..., message ordering..).

using System;
using System.Diagnostics;
using System.Threading.Tasks;
using StackExchange.Redis;

namespace ThreadExhaustionIssue {
    public class Program {
        public static async Task Main(string[] args) {
            Console.WriteLine("Connecting to Redis");
            var muxer = ConnectionMultiplexer.Connect("localhost,abortConnect=false");
            muxer.PreserveAsyncOrder = false;

            Console.WriteLine($"Current Thread Count: {Process.GetCurrentProcess().Threads.Count}");

            var subscriber = muxer.GetSubscriber();
            await subscriber.SubscribeAsync("test", OnMessage);
            for (int i = 0; i < 100; i++)
                await subscriber.PublishAsync("test", i, CommandFlags.FireAndForget);

            Console.ReadKey();
        }

        private static void OnMessage(RedisChannel channel, RedisValue value) {
            // Simulate invalidating a cache or other async operation...
            Task.Delay(10).ConfigureAwait(false).GetAwaiter().GetResult();
            Console.WriteLine($"Value: {value} Thread Count: {Process.GetCurrentProcess().Threads.Count}");
        }
    }
}

With PreserveAsyncOrder=false

Connecting to Redis
Current Thread Count: 21
Value: 2 Thread Count: 22
Value: 4 Thread Count: 23
Value: 6 Thread Count: 24
Value: 7 Thread Count: 24
Value: 9 Thread Count: 25
Value: 11 Thread Count: 26
...
...
Value: 75 Thread Count: 56
Value: 77 Thread Count: 57
Value: 79 Thread Count: 58
Value: 81 Thread Count: 59
Value: 83 Thread Count: 60

With PreserveAsyncOrder=true

Connecting to Redis
Current Thread Count: 21
Value: 0 Thread Count: 21
Value: 1 Thread Count: 21
Value: 2 Thread Count: 21
Value: 3 Thread Count: 21
Value: 4 Thread Count: 21
Value: 5 Thread Count: 21
Value: 6 Thread Count: 21
....
Value: 83 Thread Count: 21
Value: 84 Thread Count: 21
Value: 85 Thread Count: 21
Value: 86 Thread Count: 21

[runxc1]

What is the default value for PreserveAsyncOrder? Does it default to true or false?

[niemyjski]

Should be false if you don't care about message ordering (performance) I'm not sure I think it's true by default

[NickCraver]

I'm afraid that id you're using .Result (or equivalents) which are blocking threads: yes, that's going to explode the pool. The only real solution there is to go full async or use fire and forget style handling. This should be improved in 2.1 due to improvements on the .NET Core side with Task in general (thanks to mainly Toub and Adams!), but the core of the issue here is: what you're seeing is a problem with async -> sync usage attempted here.

[Yonisha]

I've just updated to 1.2.6 and it looks like the default is 'true', although the latest source code default to false.

@niemyjski do you performance improvement when set to false or still encounter performance degradation?

[AmitSuri]

PreserveAsyncOrder = true makes Assert.ThrowsAsync code hang for me.

Environment: .netcore 2.1
Bug in nUnit tests only when using SE Redis.
I am using Redis as my In memory Unit testing library.

 [Test]
        public async Task ConstructActionThrowsExceptionWhenDoingXYZ()
        {
            await CustomSetup().ConfigureAwait(false);
            Assert.ThrowsAsync<WellControllerException>(async () =>
            {
                await someController.ConstructSomething().ConfigureAwait(false);
            });
        }

If in my "someController" I use SE Redis with PreserveAsyncOrder = true. It deadlocks.
My entire code base is fully awaited..no blocking code anywhere.
I think the OP's issue is related to mine. Please correct me if I am wrong.
Ps: do let me know if this is not the right place to comment as well.

This is similar to this issue:
https://stackoverflow.com/questions/27258984/stackexchange-redis-async-call-hangs

[niemyjski]

Yes, I see degradation in both cases, but this shows what happens in real world apps. We really need an async on message handler as real life work loads use this to do things like invalidate cache and the like. I had to build a task queue to fix this issue.

[runxc1]

@niemyjski do you have an example of the task queue that you can share?

[niemyjski]

https://github.com/FoundatioFx/Foundatio/blob/master/src/Foundatio/Queues/TaskQueue.cs and https://github.com/FoundatioFx/Foundatio/blob/master/test/Foundatio.Tests/Queue/TaskQueueTests.cs

[Yonisha]

Since I set it to false I see thread pool exhaustion as reported here. Setting it back to true resolved the issue

[mgravell]

FYI I intend to look at this in the 2.0 work; we already have a bespoke pool there... Amaya as well put it to good use!

[mgravell]

Note: if you're using TLS (cloud hosts, usually) or "not .NET framework on Windows", then it is also possible that the main cause here is the IO code strategy. This has been *completely rewritten* for 2.0. I'd be very interested to know whether the above two options apply to people suffering from this. Meaning: are you using TLS or "not .NET on Windows"?

…

On Wed, 4 Jul 2018, 20:12 Yoni Shalom, ***@***.***> wrote: Since I set it to false I see thread pool exhaustion as reported here. Setting it back to true resolved the issue — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#759 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDsFEx8fNv0FdAMynnuJO8bqW8vDQ6ks5uDROTgaJpZM4RI8vH> .

[mgravell]

There is a new API in 2.0 which provides everything you'd want here for the ordered scenario (in old terms: PreserveAsyncOrder=true). In particular, it has support for async handlers, and deals with back queues internally.

I'm interested in the use-case for unordered here. We have options. It sounds like you want to use async handlers, which suggests we're waiting for them in some way; does that mean we're actually talking about something like max concurrency? or...?

For example, are you looking for something like:

process the items in any order, but with max concurrency of 8, using this async (Func<...,Task>) handler to determine completeness

?

Because: this type of thing would actually be possible for us to add now, in the 2.0 code.

If you don't care about max concurrency, then... frankly, make your existing handler async void and remove the .Wait() / .Result - it'll be properly async without blocking the thread pool.

[mgravell]

I'm going to go ahead and assume that the 2.0 changes essentially "fix" this; it completely isolates the main pump from the completion queue, and has a separate API for ordered queues (PreserveAsyncOrder just isn't a thing any more, except as a dummy prop with a [Obsolete] on it): please see #871 for more info

[niemyjski]

Thanks, I'll test this out once 2.0 becomes available :)

[niemyjski]

Also yes, I want to handle them asyncly in my message bus / pub sub and I don't really care about order just that my thread pool doesn't blow up.

[mgravell]

See, that's a fun problem; it is easy for us to restrict you to just a limited pool, but: it is absurdly easy for async/await to end up stealing threads, usually with the effect that asp.net (or whatever) slowly does a takeover; if we hard limit you, then, you can get to the point where there is no pool thread left to service you. What we do instead is use a custom pool which tries to keep everything local, but if a backlog starts to appear, it kicks into the regular thread-pool, so there's always a backup. This does mean that in super high usage you can saturate the pool, but: if you're producing work much faster than you're processing it, that's always going to be a problem - something is going to break somewhere - and if you don't go "boom" with the threads, you'll eventually go "boom" with memory instead, when you've filled up all the available space with pending work. At some point, the only reasonable comment is "process your stuff faster, or add more worker nodes to drain it as a queue in parallel"

[niemyjski]

@mgravell with 2.0 is there going to be an async OnMessage?

[mgravell]

@niemyjski are you talking about the handler in the Subscribe / SubscribeAsync API? if so, there is a new API that you could use something like:

var channel = conn.Subscribe("foo");
while (true)
{
   var msg = await channel.ReadAsync(); 
   // do something with msg, perhaps async
}

Alternatively, you could kick off an async operation in the existing Action<RedisChannel, RedisValue> handler API. We could probably add a Func<RedisChannel, RedisValue, Task> handler overload, but : we haven't added that currently.

[niemyjski]

Yeah, I wish the SubscribeAsync allowed you to execute a task and await it (out of order or in order)

[mgravell]

But in the out-of-order case the question becomes: what is awaiting it? Why is awaiting it? What would it so after it has completed? The only useful thing the code could do would be to catch and discard an exception, so... How would it help?

Basically the new V2 API plugs this hole for in-order. For out-of-order, I have yet to see an explanation of how this would help. I'm all ears if someone can talk me through it.

[niemyjski]

Say, I get a message and now I want to invalidate cache or quickly call an external service. How do I do that async? It becomes an issue in that scenario that we solved with a task queue.. But most of the time it's just something really simple async call.

[mgravell]

You just, from your handler, call a method that is "async Task", with whatever parameters you need, and either discard the returned object (if you are confident it will never throw), or use a continuation that observes errors (if it could conceivably throw).

…

On Fri, 7 Sep 2018, 21:23 Blake Niemyjski, ***@***.***> wrote: Say, I get a message and now I want to invalidate cache or quickly call an external service. How do I do that async? It becomes an issue in that scenario that we solved with a task queue.. But most of the time it's just something really simple async call. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#759 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDsKD7G3L0upoXs87tX6hx5LE2dvOPks5uYtW8gaJpZM4RI8vH> .