Classify InvalidData dispatch exceptions correctly over ice#4559
Merged
pepone merged 3 commits intoicerpc:mainfrom Apr 27, 2026
Merged
Classify InvalidData dispatch exceptions correctly over ice#4559pepone merged 3 commits intoicerpc:mainfrom
pepone merged 3 commits intoicerpc:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes status-code classification for exceptions thrown during Slice-over-Ice dispatch so malformed client requests are reported as InvalidData (instead of being lumped into InternalError), aligning behavior with the icerpc protocol while preserving canonical Ice wire-compatibility.
Changes:
- Update the
icedispatch exception classification to mapInvalidDataExceptionandIceRpcException(TruncatedData)toStatusCode.InvalidData. - Add a parameterized regression test that asserts the classified
StatusCodeobserved by the client for these exception kinds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/IceRpc/Internal/IceProtocolConnection.cs |
Classifies dispatcher-thrown InvalidDataException and TruncatedData as InvalidData for Ice replies. |
tests/IceRpc.Tests/IceProtocolConnectionTests.cs |
Adds a regression test verifying the new exception → status-code mapping. |
| [TestCase("InvalidData", StatusCode.InvalidData)] | ||
| [TestCase("TruncatedData", StatusCode.InvalidData)] | ||
| [TestCase("Other", StatusCode.InternalError)] | ||
| public async Task Thrown_dispatch_exception_is_classified(string exceptionKind, StatusCode expectedStatusCode) |
There was a problem hiding this comment.
The test name Thrown_dispatch_exception_is_classified is misleading: the dispatcher is throwing non-DispatchException exceptions (e.g., InvalidDataException, IceRpcException) and the test verifies how they are mapped into a DispatchException status code. Consider renaming the test to reflect that it covers classification of exceptions thrown by the dispatcher (not a thrown DispatchException).
Suggested change
| public async Task Thrown_dispatch_exception_is_classified(string exceptionKind, StatusCode expectedStatusCode) | |
| public async Task Thrown_dispatcher_exception_is_classified(string exceptionKind, StatusCode expectedStatusCode) |
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.
pepone
force-pushed
the
fix/ice-dispatch-invalid-data-classification
branch
from
8607768 to
c18a047
Compare
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.
InsertCreativityHere
approved these changes
Apr 27, 2026
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.
externl
approved these changes
Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4463