From fefc94256292d53c9f52da9b621d0975726d7384 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Mon, 6 May 2024 18:59:52 -0600 Subject: [PATCH] Stop throwing from `TryGetArgumentByNameOrIndex` when using the `System.Text.Json` formatter This also adjusts how we test with ETW events turned on so that issues like this will be caught rather than swallowed. Fixes #1038 --- azure-pipelines/dotnet.yml | 14 ++++++- src/StreamJsonRpc/JsonRpcEventSource.cs | 16 ++++---- src/StreamJsonRpc/SharedUtilities.cs | 38 +++++++++++++++++++ src/StreamJsonRpc/SystemTextJsonFormatter.cs | 4 +- .../JsonRpcMessagePackLengthTests.cs | 3 +- .../StreamJsonRpc.Tests.csproj | 1 + 6 files changed, 62 insertions(+), 14 deletions(-) create mode 100644 src/StreamJsonRpc/SharedUtilities.cs diff --git a/azure-pipelines/dotnet.yml b/azure-pipelines/dotnet.yml index 3d67e1105..ef7742ebc 100644 --- a/azure-pipelines/dotnet.yml +++ b/azure-pipelines/dotnet.yml @@ -17,13 +17,23 @@ steps: condition: and(succeeded(), ${{ parameters.RunTests }}) - task: DotNetCoreCLI@2 - displayName: 🧪 dotnet test -f net472 (+EventSource) + displayName: 🧪 dotnet test -f net472 (+EventSource throw) inputs: command: test arguments: --no-build -c $(BuildConfiguration) -f net472 --filter "TestCategory!=FailsInCloudTest$(FailsOnMonoFilter)" -v n /p:CollectCoverage=true /bl:"$(Build.ArtifactStagingDirectory)/build_logs/test_net472_etw.binlog" --diag "$(Build.ArtifactStagingDirectory)/test_logs/net472_etw.txt" testRunTitle: streamjsonrpc.tests-etw (net472, $(Agent.JobName)) env: - StreamJsonRpc_TestWithEventSource: 1 + StreamJsonRpc_TestWithEventSource: 1 # allow exceptions from EventSource to propagate + condition: and(succeeded(), ne(variables['OptProf'], 'true'), eq(variables['Agent.OS'], 'Windows_NT')) + +- task: DotNetCoreCLI@2 + displayName: 🧪 dotnet test -f net472 (+EventSource production) + inputs: + command: test + arguments: --no-build -c $(BuildConfiguration) -f net472 --filter "TestCategory!=FailsInCloudTest$(FailsOnMonoFilter)" -v n /p:CollectCoverage=true /bl:"$(Build.ArtifactStagingDirectory)/build_logs/test_net472_etw.binlog" --diag "$(Build.ArtifactStagingDirectory)/test_logs/net472_etw.txt" + testRunTitle: streamjsonrpc.tests-etw (net472, $(Agent.JobName)) + env: + StreamJsonRpc_TestWithEventSource: 2 # swallow exceptions from EventSource, as is done in production condition: and(succeeded(), ne(variables['OptProf'], 'true'), eq(variables['Agent.OS'], 'Windows_NT')) - ${{ if parameters.IsOptProf }}: diff --git a/src/StreamJsonRpc/JsonRpcEventSource.cs b/src/StreamJsonRpc/JsonRpcEventSource.cs index 0925407e5..30c91c4c0 100644 --- a/src/StreamJsonRpc/JsonRpcEventSource.cs +++ b/src/StreamJsonRpc/JsonRpcEventSource.cs @@ -20,7 +20,7 @@ internal sealed class JsonRpcEventSource : EventSource /// /// The singleton instance of this event source. /// - internal static readonly JsonRpcEventSource Instance = new JsonRpcEventSource(); + internal static readonly JsonRpcEventSource Instance = new(); /// /// The event ID for the . @@ -98,9 +98,9 @@ internal sealed class JsonRpcEventSource : EventSource private const int MessageHandlerReceivedEvent = 33; /// - /// A flag indicating whether to force tracing to be on. + /// Gets the testing mode that ETW tracing is running under. /// - private static readonly bool AlwaysOn = Environment.GetEnvironmentVariable("StreamJsonRpc_TestWithEventSource") == "1"; + private static readonly SharedUtilities.EventSourceTestMode EventSourceTestingActive = SharedUtilities.GetEventSourceTestMode(); /// /// Initializes a new instance of the class. @@ -113,7 +113,7 @@ private JsonRpcEventSource() } /// - public new bool IsEnabled(EventLevel level, EventKeywords keywords) => AlwaysOn || base.IsEnabled(level, keywords); + public new bool IsEnabled(EventLevel level, EventKeywords keywords) => EventSourceTestingActive != SharedUtilities.EventSourceTestMode.None || base.IsEnabled(level, keywords); /// /// Signals the transmission of a notification. @@ -301,7 +301,7 @@ internal static string GetArgumentsString(JsonRpcRequest request) formatted = true; } } - catch + catch when (EventSourceTestingActive != SharedUtilities.EventSourceTestMode.DoNotSwallowExceptions) { // Swallow exceptions when deserializing args for ETW logging. It's never worth causing a functional failure. } @@ -339,7 +339,7 @@ internal static string GetArgumentsString(JsonRpcRequest request) formatted = true; } } - catch + catch when (EventSourceTestingActive != SharedUtilities.EventSourceTestMode.DoNotSwallowExceptions) { // Swallow exceptions when deserializing args for ETW logging. It's never worth causing a functional failure. } @@ -368,9 +368,9 @@ static void Format(StringBuilder builder, object? value) builder.Append("null"); break; case string s: - builder.Append("\""); + builder.Append('"'); builder.Append(s); - builder.Append("\""); + builder.Append('"'); break; default: builder.Append(value); diff --git a/src/StreamJsonRpc/SharedUtilities.cs b/src/StreamJsonRpc/SharedUtilities.cs new file mode 100644 index 000000000..174a696dd --- /dev/null +++ b/src/StreamJsonRpc/SharedUtilities.cs @@ -0,0 +1,38 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace StreamJsonRpc; + +/// +/// Utilities that are source-shared between the library and its tests. +/// +internal static class SharedUtilities +{ + /// + /// The various modes that can be used to test the class. + /// + internal enum EventSourceTestMode + { + /// + /// ETW events are not forced on. + /// + None, + + /// + /// ETW events are forced on and exceptions are swallowed as they would be in production. + /// + EmulateProduction, + + /// + /// ETW events are forced on and exceptions are not swallowed, allowing tests to detect errors in ETW logging. + /// + DoNotSwallowExceptions, + } + + internal static EventSourceTestMode GetEventSourceTestMode() => Environment.GetEnvironmentVariable("StreamJsonRpc_TestWithEventSource") switch + { + "1" => EventSourceTestMode.EmulateProduction, + "2" => EventSourceTestMode.DoNotSwallowExceptions, + _ => EventSourceTestMode.None, + }; +} diff --git a/src/StreamJsonRpc/SystemTextJsonFormatter.cs b/src/StreamJsonRpc/SystemTextJsonFormatter.cs index e249bb19f..c00798db0 100644 --- a/src/StreamJsonRpc/SystemTextJsonFormatter.cs +++ b/src/StreamJsonRpc/SystemTextJsonFormatter.cs @@ -516,7 +516,7 @@ public override bool TryGetArgumentByNameOrIndex(string? name, int position, Typ } break; - case JsonValueKind.Array: + case JsonValueKind.Array when position >= 0: int elementIndex = 0; foreach (JsonElement arrayElement in this.JsonArguments.Value.EnumerateArray()) { @@ -528,8 +528,6 @@ public override bool TryGetArgumentByNameOrIndex(string? name, int position, Typ } break; - default: - throw new JsonException("Unexpected value kind for arguments: " + (this.JsonArguments?.ValueKind.ToString() ?? "null")); } try diff --git a/test/StreamJsonRpc.Tests/JsonRpcMessagePackLengthTests.cs b/test/StreamJsonRpc.Tests/JsonRpcMessagePackLengthTests.cs index 44d97552a..f0881be71 100644 --- a/test/StreamJsonRpc.Tests/JsonRpcMessagePackLengthTests.cs +++ b/test/StreamJsonRpc.Tests/JsonRpcMessagePackLengthTests.cs @@ -372,9 +372,10 @@ public async Task UnionType_AsAsyncEnumerableTypeArgument() /// verbose ETW tracing would fail to deserialize arguments with the primitive formatter that deserialize just fine for the actual method dispatch. /// This test is effective because custom msgpack extensions cause the to throw an exception when deserializing. /// - [Theory, PairwiseData] + [SkippableTheory, PairwiseData] public async Task VerboseLoggingDoesNotFailWhenArgsDoNotDeserializePrimitively(bool namedArguments) { + Skip.IfNot(SharedUtilities.GetEventSourceTestMode() == SharedUtilities.EventSourceTestMode.EmulateProduction, $"This test specifically verifies behavior when the EventSource should swallow exceptions. Current mode: {SharedUtilities.GetEventSourceTestMode()}."); var server = new MessagePackServer(); this.serverRpc.AllowModificationWhileListening = true; this.serverRpc.AddLocalRpcTarget(server); diff --git a/test/StreamJsonRpc.Tests/StreamJsonRpc.Tests.csproj b/test/StreamJsonRpc.Tests/StreamJsonRpc.Tests.csproj index 667ab3db0..82fb00a07 100644 --- a/test/StreamJsonRpc.Tests/StreamJsonRpc.Tests.csproj +++ b/test/StreamJsonRpc.Tests/StreamJsonRpc.Tests.csproj @@ -8,6 +8,7 @@ +