From 874cc2e27921138d97b1edb727ede0cba9090ad3 Mon Sep 17 00:00:00 2001 From: Gino Canessa Date: Wed, 4 Sep 2024 17:33:03 -0500 Subject: [PATCH] Fix: interprocess mutex cannot be async. Add: unit tests comparing json output should compare by JObject, not string. --- .../GenerationTests.cs | 48 ++++++++++++------- ...Microsoft.Health.Fhir.CodeGen.Tests.csproj | 1 + .../Loader/PackageLoader.cs | 3 -- .../Utils/InterProcessSync.cs | 8 ++-- 4 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/Microsoft.Health.Fhir.CodeGen.Tests/GenerationTests.cs b/src/Microsoft.Health.Fhir.CodeGen.Tests/GenerationTests.cs index 7614e3809..9fb416a1b 100644 --- a/src/Microsoft.Health.Fhir.CodeGen.Tests/GenerationTests.cs +++ b/src/Microsoft.Health.Fhir.CodeGen.Tests/GenerationTests.cs @@ -18,6 +18,7 @@ using Microsoft.Health.Fhir.CodeGen.Models; using Microsoft.Health.Fhir.CodeGen.Tests.Extensions; using Microsoft.Health.Fhir.CodeGenCommon.Packaging; +using Newtonsoft.Json.Linq; using Xunit.Abstractions; namespace Microsoft.Health.Fhir.CodeGen.Tests; @@ -45,7 +46,10 @@ public GenerationTestFixture() /// Full pathname of the existing file. /// The current milliseconds. /// (Optional) True to compare lines directly, false to compare set of line hashes. - internal static void CompareGeneration(string existingPath, MemoryStream currentMS, bool compareLinesDirectly = true) + internal static void CompareGeneration( + string existingPath, + MemoryStream currentMS, + bool compareLinesDirectly = true) { // make sure the MS is up to date and at the beginning currentMS.Flush(); @@ -82,6 +86,20 @@ internal static void CompareGeneration(string existingPath, MemoryStream current version = version.Substring(0, version.IndexOf('+')); } + if (existingPath.EndsWith(".json", StringComparison.OrdinalIgnoreCase)) + { + JToken expected = JToken.Parse(File.ReadAllText(existingPath)); + + byte[] msBytes = new byte[currentMS.Length]; + currentMS.Read(msBytes).Should().NotBe(0); + + JToken actual = JToken.Parse(System.Text.Encoding.UTF8.GetString(msBytes)); + + actual.Should().BeEquivalentTo(expected); + + return; + } + using FileStream existingFS = new(existingPath, FileMode.Open, FileAccess.Read); // compare files line by line @@ -437,7 +455,7 @@ public GenerationTestsR4(GenerationTestFixture fixture, ITestOutputHelper testOu [InlineData("OpenApi-Yaml-Inline-Detailed-Candle-Filtered", "TestData/Generated/OpenApi-R4-Inline-Detailed-Candle-Filtered.yaml", "TestData/R4/CapabilityStatement-candle-local.json")] [Trait("Category", "Generation")] [Trait("FhirVersion", "R4")] - internal void TestLangR4(string langName, string filePath, string csPath) + internal void TestLangR4(string langName, string filePath, string capabilityJsonPath) { // Get the absolute path to the file string path = Path.IsPathRooted(filePath) @@ -449,11 +467,11 @@ internal void TestLangR4(string langName, string filePath, string csPath) throw new ArgumentException($"Could not find file at path: {path}"); } - csPath = string.IsNullOrEmpty(csPath) + capabilityJsonPath = string.IsNullOrEmpty(capabilityJsonPath) ? string.Empty - : Path.IsPathRooted(csPath) - ? csPath - : Path.GetRelativePath(Directory.GetCurrentDirectory(), csPath); + : Path.IsPathRooted(capabilityJsonPath) + ? capabilityJsonPath + : Path.GetRelativePath(Directory.GetCurrentDirectory(), capabilityJsonPath); bool compareLinesDirectly = true; @@ -498,18 +516,17 @@ internal void TestLangR4(string langName, string filePath, string csPath) LangOpenApi exportLang = new(); exportLang.Export(options, _loaded); - compareLinesDirectly = false; } break; case "OpenApi-Json-Inline-None-Candle-Filtered": { - if (string.IsNullOrEmpty(csPath)) + if (string.IsNullOrEmpty(capabilityJsonPath)) { throw new ArgumentException($"Missing csPath for {langName}"); } - object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", csPath); + object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", capabilityJsonPath); CapabilityStatement? cs = csObj is CapabilityStatement c ? c @@ -530,18 +547,17 @@ internal void TestLangR4(string langName, string filePath, string csPath) LangOpenApi exportLang = new(); exportLang.Export(options, _loaded); - compareLinesDirectly = false; } break; case "OpenApi-Yaml-Inline-None-Candle-Filtered": { - if (string.IsNullOrEmpty(csPath)) + if (string.IsNullOrEmpty(capabilityJsonPath)) { throw new ArgumentException($"Missing csPath for {langName}"); } - object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", csPath); + object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", capabilityJsonPath); CapabilityStatement? cs = csObj is CapabilityStatement c ? c @@ -568,12 +584,12 @@ internal void TestLangR4(string langName, string filePath, string csPath) case "OpenApi-Yaml-Inline-Names-Candle-Filtered": { - if (string.IsNullOrEmpty(csPath)) + if (string.IsNullOrEmpty(capabilityJsonPath)) { throw new ArgumentException($"Missing csPath for {langName}"); } - object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", csPath); + object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", capabilityJsonPath); CapabilityStatement? cs = csObj is CapabilityStatement c ? c @@ -600,12 +616,12 @@ internal void TestLangR4(string langName, string filePath, string csPath) case "OpenApi-Yaml-Inline-Detailed-Candle-Filtered": { - if (string.IsNullOrEmpty(csPath)) + if (string.IsNullOrEmpty(capabilityJsonPath)) { throw new ArgumentException($"Missing csPath for {langName}"); } - object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", csPath); + object? csObj = _fixture.Loader?.ParseContents43("application/fhir+json", capabilityJsonPath); CapabilityStatement? cs = csObj is CapabilityStatement c ? c diff --git a/src/Microsoft.Health.Fhir.CodeGen.Tests/Microsoft.Health.Fhir.CodeGen.Tests.csproj b/src/Microsoft.Health.Fhir.CodeGen.Tests/Microsoft.Health.Fhir.CodeGen.Tests.csproj index 8887aed7f..bf8a5dd6b 100644 --- a/src/Microsoft.Health.Fhir.CodeGen.Tests/Microsoft.Health.Fhir.CodeGen.Tests.csproj +++ b/src/Microsoft.Health.Fhir.CodeGen.Tests/Microsoft.Health.Fhir.CodeGen.Tests.csproj @@ -15,6 +15,7 @@ + diff --git a/src/Microsoft.Health.Fhir.CodeGen/Loader/PackageLoader.cs b/src/Microsoft.Health.Fhir.CodeGen/Loader/PackageLoader.cs index dde7c6f6f..25d1fe511 100644 --- a/src/Microsoft.Health.Fhir.CodeGen/Loader/PackageLoader.cs +++ b/src/Microsoft.Health.Fhir.CodeGen/Loader/PackageLoader.cs @@ -666,9 +666,6 @@ private bool LoadFromDirectory(ref DefinitionCollection? definitions, string sou Guid? syncHandle = null; - // put package installation in a named sync object so that multiple spawns do not hammer the package server - // note that we cannot use a mutex because C# async (await) does not maintain the original thread identity - // note that we cannot use a semaphore because .Net does not support named semaphores on all platforms try { string sName = "fcg-" + packageReference.Moniker; diff --git a/src/Microsoft.Health.Fhir.CodeGen/Utils/InterProcessSync.cs b/src/Microsoft.Health.Fhir.CodeGen/Utils/InterProcessSync.cs index 1317b4ced..c0b2a526a 100644 --- a/src/Microsoft.Health.Fhir.CodeGen/Utils/InterProcessSync.cs +++ b/src/Microsoft.Health.Fhir.CodeGen/Utils/InterProcessSync.cs @@ -117,7 +117,7 @@ public InterProcessSync() long maxTicks = DateTime.Now.Ticks + delay; - while (DateTime.Now.Ticks < delay) + while (DateTime.Now.Ticks < maxTicks) { await Task.Delay(100); @@ -165,7 +165,7 @@ public async Task TryWait(Guid handle, int delay = _defaultDelay) long maxTicks = DateTime.Now.Ticks + delay; - while (DateTime.Now.Ticks < delay) + while (DateTime.Now.Ticks < maxTicks) { if (_mutexRequests[handle].State == InterProcessSyncStates.Owned) { @@ -216,11 +216,11 @@ public void ReleaseLock(Guid handle) /// /// Represents the method that will be executed in the mutex management thread. /// - private async void MutexManagementThreadFunc() + private void MutexManagementThreadFunc() { while (!_disposing) { - await Task.Delay(100); + System.Threading.Thread.Sleep(100); foreach (Guid key in _mutexRequests.Keys) {