Add SignalR client log for stream item binding failure #60857
Conversation
There was a problem hiding this comment.
PR Overview
This PR adds logging support for stream item binding failures in the SignalR Java and C# clients to address an issue where the Java client closed the connection without handling such errors.
- Added tests to validate stream item binding failure handling in both Java and C# clients.
- Updated protocol and connection handling logic to log binding exceptions in streams.
- Introduced a new logger message and error handling case for stream binding failures in the core client code.
Reviewed Changes
| File | Description |
|---|---|
| src/SignalR/clients/java/signalr/test/src/main/java/com/microsoft/signalr/HubConnectionTest.java | Added test to verify logging of stream binding failures. |
| src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.Protocol.cs | New test verifies that stream binding errors are logged and the connection remains active. |
| src/SignalR/clients/java/signalr/core/src/main/java/com/microsoft/signalr/GsonHubProtocol.java | Adjusted result parsing to catch binding exceptions and convert them into stream binding failure messages. |
| src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs | Added a new case to handle StreamBindingFailureMessage and log the error accordingly. |
| src/SignalR/clients/csharp/Client.Core/src/HubConnection.Log.cs | Added a logger definition for stream binding failures. |
| src/SignalR/clients/java/signalr/core/src/main/java/com/microsoft/signalr/HubConnection.java | Introduced a new case in the receive loop for stream binding failures with proper logging. |
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
src/SignalR/clients/csharp/Client/test/UnitTests/HubConnectionTests.Protocol.cs
Show resolved
Hide resolved
| try { | ||
| result = gson.fromJson(reader, binder.getReturnType(invocationId)); | ||
| } catch (Exception ex) { | ||
| argumentBindingException = ex; |
There was a problem hiding this comment.
Do we have a way of capturing the value that failed to bind here? If so, that might be helpful to capture in the exception. Ditto with the streamToken below.
There was a problem hiding this comment.
Relying on the underlying json library to provide the error, in the case of the test I added the error would contain: java.lang.NumberFormatException: For input string: "str"
| if (invocationId == null || binder.getReturnType(invocationId) == null) { | ||
| resultToken = JsonParser.parseReader(reader); | ||
| } else { | ||
| result = gson.fromJson(reader, binder.getReturnType(invocationId)); | ||
| } | ||
| break; | ||
| case "item": |
There was a problem hiding this comment.
Is there a reason we don't need to set the same guards when we're parsing properties from the result key?
There was a problem hiding this comment.
The reason is that it's a different bug/improvement and I don't want to pollute this PR 😆
| @@ -10,11 +10,10 @@ dependencies { | |||
| implementation 'org.junit.jupiter:junit-jupiter-params:5.11.2' | |||
| testImplementation 'org.junit.jupiter:junit-jupiter:5.11.2' | |||
| testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.11.2' | |||
| implementation 'com.google.code.gson:gson:2.8.5' | |||
| implementation 'com.google.code.gson:gson:2.8.9' | |||
There was a problem hiding this comment.
Is this bump related to the fix or just for good measure?
There was a problem hiding this comment.
Shouldn't have been needed, but for some reason when debugging the test in VS Code it complained about a method not being found in gson, command line passed just fine. So I updated the dep and VS Code started working 🤷
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
/azp run |
BrennanConroy
removed
the
pending-ci-rerun
|
Azure Pipelines successfully started running 3 pipeline(s). |
Fixes #56303
Turns out the Java client didn't even handle stream binding failures and just closed the connection, so added support for that.