Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix ClientWebSocket error message when endpoint is not a real websocket #29159

Merged
merged 3 commits into from
Apr 18, 2018
Merged

Fix ClientWebSocket error message when endpoint is not a real websocket #29159

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51c551a
fix clientwebsocket error message
caesar-chen Apr 17, 2018
44eaf61
address feedback
caesar-chen Apr 17, 2018
4beae9e
fix comment
caesar-chen Apr 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/System.Net.WebSockets.Client/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@
<data name="net_WebSockets_ArgumentOutOfRange_TooSmall" xml:space="preserve">
<value>The argument must be a value greater than {0}.</value>
</data>
<data name="net_WebSockets_Connect101Expected" xml:space="preserve">
<value>The server returned status code '{0}' when status code '101' was expected.</value>
</data>
<data name="net_WebSockets_InvalidState_ClosedOrAborted" xml:space="preserve">
<value>The '{0}' instance cannot be used for communication because it has been transitioned into the '{1}' state.</value>
</data>
Expand Down
2 changes: 1 addition & 1 deletion src/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ public async Task ConnectAsyncCore(Uri uri, CancellationToken cancellationToken,

if (response.StatusCode != HttpStatusCode.SwitchingProtocols)
{
throw new WebSocketException(SR.net_webstatus_ConnectFailure);
throw new WebSocketException(SR.Format(SR.net_WebSockets_Connect101Expected, (int) response.StatusCode));
}

// The Connection, Upgrade, and SecWebSocketAccept headers are required and with specific values.
Expand Down
8 changes: 6 additions & 2 deletions src/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,23 @@ public static IEnumerable<object[]> UnavailableWebSocketServers
get
{
Uri server;
string exceptionMessage;

// Unknown server.
{
server = new Uri(string.Format("ws://{0}", Guid.NewGuid().ToString()));
yield return new object[] { server };
exceptionMessage = ResourceHelper.GetExceptionMessage("net_webstatus_ConnectFailure");

yield return new object[] { server, exceptionMessage };
}

// Known server but not a real websocket endpoint.
{
server = System.Net.Test.Common.Configuration.Http.RemoteEchoServer;
var ub = new UriBuilder("ws", server.Host, server.Port, server.PathAndQuery);
exceptionMessage = ResourceHelper.GetExceptionMessage("net_WebSockets_Connect101Expected", (int) HttpStatusCode.OK);

yield return new object[] { ub.Uri };
yield return new object[] { ub.Uri, exceptionMessage };
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions src/System.Net.WebSockets.Client/tests/ConnectTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@ public class ConnectTest : ClientWebSocketTestBase
{
public ConnectTest(ITestOutputHelper output) : base(output) { }

[ActiveIssue(20360, TargetFrameworkMonikers.NetFramework)]
[OuterLoop] // TODO: Issue #11345
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(UnavailableWebSocketServers))]
public async Task ConnectAsync_NotWebSocketServer_ThrowsWebSocketExceptionWithMessage(Uri server)
public async Task ConnectAsync_NotWebSocketServer_ThrowsWebSocketExceptionWithMessage(Uri server, string exceptionMessage)
{
using (var cws = new ClientWebSocket())
{
Expand All @@ -29,7 +28,12 @@ public async Task ConnectAsync_NotWebSocketServer_ThrowsWebSocketExceptionWithMe

Assert.Equal(WebSocketError.Success, ex.WebSocketErrorCode);
Assert.Equal(WebSocketState.Closed, cws.State);
Assert.Equal(ResourceHelper.GetExceptionMessage("net_webstatus_ConnectFailure"), ex.Message);

// The .Net Native toolchain optimizes away exception messages.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

".Net" -> ".NET"

if (!PlatformDetection.IsNetNative)
Copy link
Contributor Author

@caesar-chen caesar-chen Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to PlatformDetection instead of AssertExtension, because we also want to validate the ex.WebSocketErrorCode above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should still be able to use AssertExtension.Throws and save the exception it gives you back. And then check the error code. Using AssertExtension can remove you needing to check for "IsNetNative".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AssertExtension doesn't contain method overload to examine WebSocketException (or Exception in general) error message field and return the Exception. Which means, if we use other overload to save the Exception, we still need to use IsNetNative check in our test code to verify the ErrorMessage.

  1. It's possible to add a method overload inside AssertExtension class dedicated for WebSocketException, but the overhead is that we need to add System.Net.WebSockets reference inside the CoreFx.private.TestUtilities, and I'm not sure if that's desirable (Currently that project has no dependency on System.Net).
  2. It's not feasible to add a method overload for Exception in general that, do both check error message and return the exception. Because all possible distinct function signatures have been taken. Or we will need to add public API (new methods) for that class.

My preference is to use the current if check in our test code, if the desire for checking WebSocketException is high, we could consider add it to the common AssertExtension.

@davidsh Do you have any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for looking into this. Let's just keep it the way you're doing it in this PR.

{
Assert.Equal(exceptionMessage, ex.Message);
}
}
}

Expand Down