From 6b7b24cec11f5db28c54f14d66b600dc0193e705 Mon Sep 17 00:00:00 2001 From: Endre T Date: Thu, 7 Jan 2021 01:16:46 +0100 Subject: [PATCH] implement tests to prevent syncronizecontext deadlocks (#18) Co-authored-by: andrew-cat --- .../ConfigCatClientTests.cs | 89 ++++++++++++++ .../SynchronizationContextDeadlockTests.cs | 114 ++++++++++++++++++ src/ConfigCatClient/ConfigCatClient.cs | 52 +++++--- .../ConfigService/AutoPollConfigService.cs | 4 +- src/ConfigCatClient/HttpConfigFetcher.cs | 2 +- 5 files changed, 243 insertions(+), 18 deletions(-) create mode 100644 src/ConfigCat.Client.Tests/SynchronizationContextDeadlockTests.cs diff --git a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs b/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs index 3e69fd0a..1cfc47b3 100644 --- a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs +++ b/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs @@ -494,6 +494,95 @@ public void Dispose_ConfigServiceIsDisposable_ShouldInvokeDispose() Assert.AreEqual(1, myMock.DisposeCount); } + [TestMethod] + public void ForceRefresh_ShouldInvokeConfigServiceRefreshConfigAsync() + { + // Arrange + + configServiceMock.Setup(m => m.RefreshConfigAsync()).Returns(Task.CompletedTask); + + IConfigCatClient instance = new ConfigCatClient( + configServiceMock.Object, + loggerMock.Object, + evaluatorMock.Object, + configDeserializerMock.Object); + + // Act + + instance.ForceRefresh(); + + // Assert + + configServiceMock.Verify(m => m.RefreshConfigAsync(), Times.Once); + } + + [TestMethod] + public async Task ForceRefreshAsync_ShouldInvokeConfigServiceRefreshConfigAsync() + { + // Arrange + + configServiceMock.Setup(m => m.RefreshConfigAsync()).Returns(Task.CompletedTask); + + IConfigCatClient instance = new ConfigCatClient( + configServiceMock.Object, + loggerMock.Object, + evaluatorMock.Object, + configDeserializerMock.Object); + + // Act + + await instance.ForceRefreshAsync(); + + // Assert + + configServiceMock.Verify(m => m.RefreshConfigAsync(), Times.Once); + } + + + [TestMethod] + public void ForceRefresh_ConfigServiceThrowException_ShouldNotReThrowTheExceptionAndLogsError() + { + // Arrange + + configServiceMock.Setup(m => m.RefreshConfigAsync()).Throws(); + + IConfigCatClient instance = new ConfigCatClient( + configServiceMock.Object, + loggerMock.Object, + evaluatorMock.Object, + configDeserializerMock.Object); + + // Act + + instance.ForceRefresh(); + + // Assert + + loggerMock.Verify(m => m.Error(It.IsAny()), Times.Once); + } + + [TestMethod] + public async Task ForceRefreshAsync_ConfigServiceThrowException_ShouldNotReThrowTheExceptionAndLogsError() + { + // Arrange + + configServiceMock.Setup(m => m.RefreshConfigAsync()).Throws(); + + IConfigCatClient instance = new ConfigCatClient( + configServiceMock.Object, + loggerMock.Object, + evaluatorMock.Object, + configDeserializerMock.Object); + + // Act + + await instance.ForceRefreshAsync(); + + // Assert + + loggerMock.Verify(m => m.Error(It.IsAny()), Times.Once); + } + internal class FakeConfigService : ConfigServiceBase, IConfigService { public byte DisposeCount { get; private set; } diff --git a/src/ConfigCat.Client.Tests/SynchronizationContextDeadlockTests.cs b/src/ConfigCat.Client.Tests/SynchronizationContextDeadlockTests.cs new file mode 100644 index 00000000..f4412a18 --- /dev/null +++ b/src/ConfigCat.Client.Tests/SynchronizationContextDeadlockTests.cs @@ -0,0 +1,114 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System; +using System.Threading; +using Moq; +using System.Linq; +using System.Reflection; +using System.Threading.Tasks; + +namespace ConfigCat.Client.Tests +{ + [TestClass] + public class SynchronizationContextDeadlockTests + { + private const string SDKKEY = "PKDVCLf-Hq-h-kCzMp-L7Q/psuH7BGHoUmdONrzzUOY7A"; + + private readonly Mock syncContextMock; + + private static SynchronizationContext synchronizationContextBackup; + + public SynchronizationContextDeadlockTests() + { + synchronizationContextBackup = SynchronizationContext.Current; + + syncContextMock = new Mock + { + CallBase = true + }; + + SynchronizationContext.SetSynchronizationContext(syncContextMock.Object); + } + + [ClassCleanup] + public static void ClassCleanup() + { + SynchronizationContext.SetSynchronizationContext(synchronizationContextBackup); + } + + [TestInitialize] + public void TestInitialize() + { + syncContextMock.Reset(); + } + + [TestMethod] + public void AutoPollDeadLockCheck() + { + var client = new ConfigCatClient(new AutoPollConfiguration + { + SdkKey = SDKKEY, + Logger = new ConsoleLogger(LogLevel.Off) + }); + + ClientDeadlockCheck(client); + } + + [TestMethod] + public void ManualPollDeadLockCheck() + { + var client = new ConfigCatClient(new ManualPollConfiguration + { + SdkKey = SDKKEY, + Logger = new ConsoleLogger(LogLevel.Off) + }); + + ClientDeadlockCheck(client); + } + + [TestMethod] + public void LazyLoadDeadLockCheck() + { + var client = new ConfigCatClient(new LazyLoadConfiguration + { + SdkKey = SDKKEY, + Logger = new ConsoleLogger(LogLevel.Off) + }); + + ClientDeadlockCheck(client); + } + + private void ClientDeadlockCheck(IConfigCatClient client) + { + var methods = typeof(IConfigCatClient).GetMethods().Where(x => !x.IsSpecialName).OrderBy(o => o.Name); + + foreach (var m in methods) + { + var parameters = Enumerable.Repeat(null, m.GetParameters().Length).ToArray(); + + MethodInfo mi = m; + + if (m.IsGenericMethod) + { + mi = m.MakeGenericMethod(typeof(string)); + } + + Console.WriteLine($"Invoke '{mi.Name}' method"); + + if (mi.ReturnType.IsSubclassOf(typeof(Task))) + { + var task = (Task)mi.Invoke(client, parameters); + + task.ConfigureAwait(false); + task.Wait(); + } + else + { + mi.Invoke(client, parameters); + } + + syncContextMock.Verify(x => x.Post(It.IsAny(), It.IsAny()), Times.Never, $"Method: {mi.Name}"); + syncContextMock.Verify(x => x.Send(It.IsAny(), It.IsAny()), Times.Never, $"Method: {mi.Name}"); + } + } + } +} diff --git a/src/ConfigCatClient/ConfigCatClient.cs b/src/ConfigCatClient/ConfigCatClient.cs index 46b15933..f8f17424 100644 --- a/src/ConfigCatClient/ConfigCatClient.cs +++ b/src/ConfigCatClient/ConfigCatClient.cs @@ -143,9 +143,9 @@ public T GetValue(string key, T defaultValue, User user = null) { try { - var c = this.configService.GetConfigAsync().Result; + var config = this.configService.GetConfigAsync().GetAwaiter().GetResult(); - return this.configEvaluator.Evaluate(c, key, defaultValue, user); + return this.configEvaluator.Evaluate(config, key, defaultValue, user); } catch (Exception ex) { @@ -160,9 +160,9 @@ public async Task GetValueAsync(string key, T defaultValue, User user = nu { try { - var c = await this.configService.GetConfigAsync().ConfigureAwait(false); + var config = await this.configService.GetConfigAsync().ConfigureAwait(false); - return this.configEvaluator.Evaluate(c, key, defaultValue, user); + return this.configEvaluator.Evaluate(config, key, defaultValue, user); } catch (Exception ex) { @@ -177,7 +177,13 @@ public IEnumerable GetAllKeys() { try { - return this.GetAllKeysAsync().Result; + var config = this.configService.GetConfigAsync().GetAwaiter().GetResult(); + + if (this.configDeserializer.TryDeserialize(config, out var settings)) return settings.Keys; + + this.log.Warning("Config deserialization failed."); + + return new string[0]; } catch (Exception ex) { @@ -191,10 +197,12 @@ public async Task> GetAllKeysAsync() { try { - var c = await this.configService.GetConfigAsync().ConfigureAwait(false); - if (this.configDeserializer.TryDeserialize(c, out var settings)) return settings.Keys; + var config = await this.configService.GetConfigAsync().ConfigureAwait(false); + + if (this.configDeserializer.TryDeserialize(config, out var settings)) return settings.Keys; this.log.Warning("Config deserialization failed."); + return new string[0]; } catch (Exception ex) @@ -207,13 +215,27 @@ public async Task> GetAllKeysAsync() /// public void ForceRefresh() { - this.configService.RefreshConfigAsync().Wait(); + try + { + this.configService.RefreshConfigAsync().GetAwaiter().GetResult(); + } + catch (Exception ex) + { + this.log.Error($"Error occured in 'ForceRefresh' method.\n{ex}"); + } } /// public async Task ForceRefreshAsync() { - await this.configService.RefreshConfigAsync(); + try + { + await this.configService.RefreshConfigAsync().ConfigureAwait(false); + } + catch (Exception ex) + { + this.log.Error($"Error occured in 'ForceRefreshAsync' method.\n{ex}"); + } } /// @@ -230,9 +252,9 @@ public string GetVariationId(string key, string defaultVariationId, User user = { try { - var c = this.configService.GetConfigAsync().Result; + var config = this.configService.GetConfigAsync().GetAwaiter().GetResult(); - return this.configEvaluator.EvaluateVariationId(c, key, defaultVariationId, user); + return this.configEvaluator.EvaluateVariationId(config, key, defaultVariationId, user); } catch (Exception ex) { @@ -264,9 +286,9 @@ public IEnumerable GetAllVariationId(User user = null) { try { - var c = this.configService.GetConfigAsync().Result; + var config = this.configService.GetConfigAsync().GetAwaiter().GetResult(); - return GetAllVariationIdLogic(c, user); + return GetAllVariationIdLogic(config, user); } catch (Exception ex) { @@ -281,9 +303,9 @@ public async Task> GetAllVariationIdAsync(User user = null) { try { - var c = await this.configService.GetConfigAsync().ConfigureAwait(false); + var config = await this.configService.GetConfigAsync().ConfigureAwait(false); - return GetAllVariationIdLogic(c, user); + return GetAllVariationIdLogic(config, user); } catch (Exception ex) { diff --git a/src/ConfigCatClient/ConfigService/AutoPollConfigService.cs b/src/ConfigCatClient/ConfigService/AutoPollConfigService.cs index 57c8a607..fc388b5a 100644 --- a/src/ConfigCatClient/ConfigService/AutoPollConfigService.cs +++ b/src/ConfigCatClient/ConfigService/AutoPollConfigService.cs @@ -67,7 +67,7 @@ public async Task GetConfigAsync() { if (!initializedEventSlim.Wait(delay)) { - await RefreshLogicAsync("init"); + await RefreshLogicAsync("init").ConfigureAwait(false); } cacheConfig = await this.configCache.GetAsync(base.cacheKey).ConfigureAwait(false); @@ -78,7 +78,7 @@ public async Task GetConfigAsync() public async Task RefreshConfigAsync() { - await RefreshLogicAsync("manual"); + await RefreshLogicAsync("manual").ConfigureAwait(false); } private async Task RefreshLogicAsync(object sender) diff --git a/src/ConfigCatClient/HttpConfigFetcher.cs b/src/ConfigCatClient/HttpConfigFetcher.cs index e0b95526..4c9551f0 100644 --- a/src/ConfigCatClient/HttpConfigFetcher.cs +++ b/src/ConfigCatClient/HttpConfigFetcher.cs @@ -45,7 +45,7 @@ public async Task Fetch(ProjectConfig lastConfig) try { - var fetchResult = await FetchRequest(lastConfig, this.requestUri); + var fetchResult = await FetchRequest(lastConfig, this.requestUri).ConfigureAwait(false); var response = fetchResult.Item1;