Fix silent hang in message processing loop when deserialization fails#15578
Fix silent hang in message processing loop when deserialization fails#15578nohwnd wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a silent hang by ensuring that when request/message processing fails (notably during deserialization), the failing side proactively notifies the peer via a ProtocolError message and then shuts down the session/loop.
Changes:
- Wraps
TestRequestHandler.OnMessageReceivedin a try/catch and sendsProtocolErrorbefore closing the testhost session. - Updates
DesignModeClient.ProcessRequestsexception handling to sendProtocolErrorto the IDE before stopping the design-mode client loop.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Microsoft.TestPlatform.CrossPlatEngine/EventHandlers/TestRequestHandler.cs | Adds a guarded message handler that sends ProtocolError on exceptions and closes the session. |
| src/Microsoft.TestPlatform.Client/DesignMode/DesignModeClient.cs | Sends ProtocolError to the IDE when request processing fails before stopping the client loop. |
| } | ||
| catch (Exception sendEx) | ||
| { | ||
| EqtTrace.Error("TestRequestHandler.OnMessageReceived: Failed to send ProtocolError: {0}", sendEx); |
There was a problem hiding this comment.
The PR description calls out failures due to missing System.Text.Json dependencies during deserialization. In that failure mode, _dataSerializer.SerializePayload(...) is likely to fail for the same reason, which would prevent ProtocolError from being sent and can reintroduce the hang. Consider a fallback error-notification path that does not depend on the JSON serializer (e.g., sending a minimal pre-serialized frame / raw transport-level error message if supported), or ensure the communication layer can emit ProtocolError even when payload serialization fails.
| EqtTrace.Error("TestRequestHandler.OnMessageReceived: Failed to send ProtocolError: {0}", sendEx); | |
| EqtTrace.Error("TestRequestHandler.OnMessageReceived: Failed to send ProtocolError using serializer: {0}", sendEx); | |
| // Fallback: attempt to send a minimal, serializer-independent error message | |
| try | |
| { | |
| var fallbackMessage = "ProtocolError: " + ex.ToString(); | |
| EqtTrace.Verbose("TestRequestHandler.OnMessageReceived: Sending fallback ProtocolError message without serializer: {0}", fallbackMessage); | |
| SendData(fallbackMessage); | |
| } | |
| catch (Exception fallbackEx) | |
| { | |
| EqtTrace.Error("TestRequestHandler.OnMessageReceived: Failed to send fallback ProtocolError message: {0}", fallbackEx); | |
| } |
| // waiting for a response that will never come. | ||
| try | ||
| { | ||
| _communicationManager.SendMessage(MessageType.ProtocolError, ex.Message); |
There was a problem hiding this comment.
Using ex.Message alone can be empty and loses the exception type/context, which makes the ProtocolError less actionable for the IDE/user. Consider including at least the exception type + message (e.g., ${ex.GetType().FullName}: ${ex.Message}) or a structured error payload, while being mindful of not sending overly large data (stack traces) over the protocol unless explicitly desired.
| _communicationManager.SendMessage(MessageType.ProtocolError, ex.Message); | |
| _communicationManager.SendMessage( | |
| MessageType.ProtocolError, | |
| string.Format( | |
| CultureInfo.InvariantCulture, | |
| "{0}: {1}", | |
| ex.GetType().FullName, | |
| ex.Message)); |
| // Notify the caller (IDE) about the error so it does not hang | ||
| // waiting for a response that will never come. | ||
| try | ||
| { | ||
| _communicationManager.SendMessage(MessageType.ProtocolError, ex.Message); | ||
| } | ||
| catch (Exception sendEx) | ||
| { | ||
| EqtTrace.Error("DesignModeClient: Failed to send ProtocolError to client: {0}", sendEx); | ||
| } |
There was a problem hiding this comment.
This introduces a repeated pattern (log error → try send ProtocolError → log send failure). Since similar logic now exists on both sides (testhost + vstest.console), consider extracting a small helper (per component) like TrySendProtocolError(Exception ex) to keep exception paths consistent and reduce future drift (e.g., message formatting, telemetry, or fallback behavior).
|
does not make much sense to send message to caller when we most likely cannot communicate with them. |
When message processing throws an exception (e.g. during deserialization when System.Text.Json dependencies cannot be loaded), the message loop would either silently exit or swallow the error, leaving the caller hanging indefinitely waiting for a response that never comes. Fix both DesignModeClient.ProcessRequests (vstest.console side) and TestRequestHandler.OnMessageReceived (testhost side) to send a ProtocolError message back to the caller before stopping, so the caller gets notified of the error instead of hanging. Closes microsoft#15577 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2d1ad3f to
764b674
Compare
0bf3ee2 to
ad81f8a
Compare
When message processing throws an exception (e.g. during deserialization when System.Text.Json dependencies cannot be loaded), the message loop would either silently exit or swallow the error, leaving the caller hanging indefinitely waiting for a response that never comes.
This fix updates both DesignModeClient.ProcessRequests (vstest.console side) and TestRequestHandler.OnMessageReceived (testhost side) to send a
ProtocolErrormessage back to the caller before stopping, so the caller gets notified of the error instead of hanging.Closes #15577