Possible deadlock / performance degradation in StatsdClientExtensions (.NET Full Framework)

[ARamsden]

Hi there,

Overview

While running some load tests on a windows service (.NET 4.7.1) that makes use of this library, an issue was found where processing of requests were taking a really long time. All I had to investigate was a process dump, and when looking at this, it seemed as though the call to the statsdClientExtensions was holding the threads up. I disabled the logging of the stats and the service ran smoothly under load.

Investigation

I wrote a small application to try and reproduce this, and while I could not get it to deadlock, I did notice that stats did not make it to graphite and there was a significant performance impact.

1. I noticed that the statsdClientExtensions calls async methods from a synchronous context using The Blocking Hack, which is susceptible to deadlocks.
2. I decided to try resolve this by implementing The Thread Pool Hack (specifically the version that makes use of the AsyncContext.Run().
3. This does not seem to be an issue in .NET Core,

Sample App

static async Task Main(string[] args)
{
  var statsD  = new Statsd("localhost", 12000, ConnectionType.Udp);
  var tasks = new Task[500];

  for (int i = 0; i < 500; i++)
  {
    tasks[i] = Task.Run(() => statsD.LogTiming("Andrew.TestingStats.FullPlayground.ProcessingTime", 30000));
  }

  await Task.WhenAll(tasks).ConfigureAwait(false);

  Console.WriteLine("Done.");
  Console.ReadLine();
}

Results

Below you can see that in the .NET4.7.1 example, Only +/- 60 of the 500 stats made it through and the performance was worse than that of the other 2 runs.

.NET4.7.1 Performance Results (dotTrace) - Original Implementation

Below you can see that the majority of the time is sitting waiting.

.NET4.7.1 Performance Results (dotTrace) - Attempted Fix

.NET Core Performance Results (dotTrace) - Original Implementation

Conclusion

Based on the above, it does seem as though The Blocking Hack is less than ideal when running in a Full Framework application. There seem to be a number of ways to achieve the requirement of calling async code from a sync context: https://docs.microsoft.com/en-us/archive/msdn-magazine/2015/july/async-programming-brownfield-async-development. I'm in favour of going with Thread Pool Hack using the AsyncContext.Run(). My other option (as a consumer) would of course be to revert back to using v1.3 which does not have the Async support.

I'd be keen to get your thoughts on this.

Thanks, Andrew

[lukevenediger]

Hey @ARamsden - thanks for logging this issue and really appreciate you adding so much detail. I like your suggestion of using the Thread Pool Hack - would you mind trying it out and seeing if that works for you? Is it something that we'd want to enable / disable with a flag?

If that solution works for you then please fire off a PR and we'll bring it in.

Thanks! Luke

[ARamsden]

Hey @lukevenediger,

I've only just got around to looking more into this. While the Thread Pool Hack does solve the immediate issue, it does seem to have a disadvantage which is a bit of a deal breaker for me, given the high throughput of my application. "Of course, there’s a disadvantage to this approach, as well. The thread pool thread is blocked within the AsyncContext until the asynchronous method completes. This blocked thread is there, as well as the primary thread calling the synchronous API. So, for the duration of the call, there are two threads being blocked. On ASP.NET in particular, this approach will significantly reduce the application’s ability to scale." I think the cleaner approach would be to introduce pure async extension methods that can be called from consumer's async methods, this would ensure async throughout the call chain. We could then either keep the current implementation of the sync extension methods (using the Blocking Hack) or still implement the Thread Pool Hack for safety.

I'm currently making the changes, and should have a PR soon.