From c18a047d90b86e6682a5426278605692df5aa6d3 Mon Sep 17 00:00:00 2001 From: Jose Date: Mon, 27 Apr 2026 15:25:31 +0200 Subject: [PATCH 1/3] Classify InvalidData dispatch exceptions correctly over ice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The icerpc dispatcher catch block already maps InvalidDataException to StatusCode.InvalidData. The ice dispatcher catch block had no such switch — every non-DispatchException ended up as InternalError, so a malformed Slice-over-ice request was reported to the client as a server fault and inflated server-error rates on dashboards that split client- vs server-side faults by status code. Map InvalidDataException to StatusCode.InvalidData. The ice encoder's existing case at IceProtocolConnection.cs:896-898 emits a real ReplyStatus.InvalidData on the wire, so canonical-Ice peers see the right status. Add a parameterized regression test asserting that a thrown InvalidDataException surfaces as StatusCode.InvalidData and an unrelated thrown exception still becomes StatusCode.InternalError. --- src/IceRpc/Internal/IceProtocolConnection.cs | 4 ++- .../IceProtocolConnectionTests.cs | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/IceRpc/Internal/IceProtocolConnection.cs b/src/IceRpc/Internal/IceProtocolConnection.cs index cb32053239..4201a3fd1d 100644 --- a/src/IceRpc/Internal/IceProtocolConnection.cs +++ b/src/IceRpc/Internal/IceProtocolConnection.cs @@ -1052,7 +1052,9 @@ private async Task DispatchRequestAsync(IncomingRequest request, int requestId, { if (exception is not DispatchException dispatchException) { - dispatchException = new DispatchException(StatusCode.InternalError, innerException: exception); + StatusCode statusCode = exception is InvalidDataException ? + StatusCode.InvalidData : StatusCode.InternalError; + dispatchException = new DispatchException(statusCode, innerException: exception); } response = dispatchException.ToOutgoingResponse(request); } diff --git a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs index ad2ecb0bbe..0fa84c9df3 100644 --- a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs +++ b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs @@ -145,6 +145,36 @@ public async Task Dispatcher_failure( Assert.That(readResult.Buffer.IsEmpty, Is.True); } + /// Verifies that exceptions thrown by the dispatcher are classified into the right status code: + /// InvalidDataException surfaces as StatusCode.InvalidData, anything else as StatusCode.InternalError. + [TestCase("InvalidData", StatusCode.InvalidData)] + [TestCase("Other", StatusCode.InternalError)] + public async Task Thrown_dispatch_exception_is_classified(string exceptionKind, StatusCode expectedStatusCode) + { + // Arrange + var dispatcher = new InlineDispatcher((request, cancellationToken) => exceptionKind switch + { + "InvalidData" => throw new InvalidDataException("boom"), + _ => throw new InvalidOperationException("boom") + }); + + await using ServiceProvider provider = new ServiceCollection() + .AddProtocolTest(Protocol.Ice, dispatcher) + .BuildServiceProvider(validateScopes: true); + var sut = provider.GetRequiredService(); + await sut.ConnectAsync(); + using var request = new OutgoingRequest(new ServiceAddress(Protocol.Ice) { Path = "/foo" }) + { + Operation = "op" + }; + + // Act + IncomingResponse response = await sut.Client.InvokeAsync(request); + + // Assert + Assert.That(response.StatusCode, Is.EqualTo(expectedStatusCode)); + } + /// Verifies that a StatusCode dispatched by the server is encoded as a ReplyStatus and decoded back to /// the expected StatusCode by the client. [Test, TestCaseSource(nameof(StatusCodeRoundTripSource))] From 0aeeadcc52678160d088bbe675201f47b6d1d790 Mon Sep 17 00:00:00 2001 From: Jose Date: Mon, 27 Apr 2026 15:43:06 +0200 Subject: [PATCH 2/3] Rename test to clarify it covers dispatcher-thrown exceptions Per review, "dispatch exception" reads like the DispatchException type. The test exercises non-DispatchException exceptions thrown by the dispatcher and verifies how the catch block classifies them into a DispatchException's status code, so "dispatcher exception" is more accurate. --- tests/IceRpc.Tests/IceProtocolConnectionTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs index 0fa84c9df3..b70cefad6e 100644 --- a/tests/IceRpc.Tests/IceProtocolConnectionTests.cs +++ b/tests/IceRpc.Tests/IceProtocolConnectionTests.cs @@ -149,7 +149,7 @@ public async Task Dispatcher_failure( /// InvalidDataException surfaces as StatusCode.InvalidData, anything else as StatusCode.InternalError. [TestCase("InvalidData", StatusCode.InvalidData)] [TestCase("Other", StatusCode.InternalError)] - public async Task Thrown_dispatch_exception_is_classified(string exceptionKind, StatusCode expectedStatusCode) + public async Task Thrown_dispatcher_exception_is_classified(string exceptionKind, StatusCode expectedStatusCode) { // Arrange var dispatcher = new InlineDispatcher((request, cancellationToken) => exceptionKind switch From eea4f05be104a068298844a7be96c87146c2aaf6 Mon Sep 17 00:00:00 2001 From: Jose Date: Mon, 27 Apr 2026 17:43:13 +0200 Subject: [PATCH 3/3] Update Dispatcher_throws_exception expectation for ice + InvalidData This test case codified the pre-fix behavior (ice dispatcher mapping InvalidDataException to InternalError). With the catch-block change in this PR, both protocols now classify thrown InvalidDataException as StatusCode.InvalidData, so update the expectation to match. --- tests/IceRpc.Tests/ProtocolConnectionTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/IceRpc.Tests/ProtocolConnectionTests.cs b/tests/IceRpc.Tests/ProtocolConnectionTests.cs index 7ddcf9af19..2bf48bec76 100644 --- a/tests/IceRpc.Tests/ProtocolConnectionTests.cs +++ b/tests/IceRpc.Tests/ProtocolConnectionTests.cs @@ -33,7 +33,7 @@ private static IEnumerable DispatcherThrowsExceptionSource } yield return new(Protocol.IceRpc, new InvalidDataException("invalid data"), StatusCode.InvalidData); - yield return new(Protocol.Ice, new InvalidDataException("invalid data"), StatusCode.InternalError); + yield return new(Protocol.Ice, new InvalidDataException("invalid data"), StatusCode.InvalidData); yield return new( Protocol.IceRpc,