Skip to content

Websocket handshake perf #12386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Nov 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions src/Middleware/Middleware.sln
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,10 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Server
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.SpaServices.Extensions.Tests", "SpaServices.Extensions\test\Microsoft.AspNetCore.SpaServices.Extensions.Tests.csproj", "{D0CB733B-4CE8-4F6C-BBB9-548EA1A96966}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.WebSockets.Microbenchmarks", "perf\Microbenchmarks\Microsoft.AspNetCore.WebSockets.Microbenchmarks.csproj", "{C4D624B3-749E-41D8-A43B-B304BC3885EA}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Perf", "Perf", "{4623F52E-2070-4631-8DEE-7D2F48733FFD}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -1599,6 +1603,18 @@ Global
{D0CB733B-4CE8-4F6C-BBB9-548EA1A96966}.Release|x64.Build.0 = Release|Any CPU
{D0CB733B-4CE8-4F6C-BBB9-548EA1A96966}.Release|x86.ActiveCfg = Release|Any CPU
{D0CB733B-4CE8-4F6C-BBB9-548EA1A96966}.Release|x86.Build.0 = Release|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Debug|Any CPU.Build.0 = Debug|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Debug|x64.ActiveCfg = Debug|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Debug|x64.Build.0 = Debug|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Debug|x86.ActiveCfg = Debug|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Debug|x86.Build.0 = Debug|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Release|Any CPU.ActiveCfg = Release|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Release|Any CPU.Build.0 = Release|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Release|x64.ActiveCfg = Release|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Release|x64.Build.0 = Release|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Release|x86.ActiveCfg = Release|Any CPU
{C4D624B3-749E-41D8-A43B-B304BC3885EA}.Release|x86.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down Expand Up @@ -1725,6 +1741,7 @@ Global
{46B4FE62-06A1-4D54-B3E8-D8B4B3560075} = {ACA6DDB9-7592-47CE-A740-D15BF307E9E0}
{92E11EBB-759E-4DA8-AB61-A9977D9F97D0} = {ACA6DDB9-7592-47CE-A740-D15BF307E9E0}
{D0CB733B-4CE8-4F6C-BBB9-548EA1A96966} = {D6FA4ABE-E685-4EDD-8B06-D8777E76B472}
{C4D624B3-749E-41D8-A43B-B304BC3885EA} = {4623F52E-2070-4631-8DEE-7D2F48733FFD}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {83786312-A93B-4BB4-AB06-7C6913A59AFA}
Expand Down
45 changes: 27 additions & 18 deletions src/Middleware/WebSockets/src/HandshakeHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ internal static class HandshakeHelpers
HeaderNames.SecWebSocketVersion
};

// "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"
private static ReadOnlySpan<byte> _encodedWebSocketKey => new byte[]
{
(byte)'2', (byte)'5', (byte)'8', (byte)'E', (byte)'A', (byte)'F', (byte)'A', (byte)'5', (byte)'-',
(byte)'E', (byte)'9', (byte)'1', (byte)'4', (byte)'-', (byte)'4', (byte)'7', (byte)'D', (byte)'A',
(byte)'-', (byte)'9', (byte)'5', (byte)'C', (byte)'A', (byte)'-', (byte)'C', (byte)'5', (byte)'A',
(byte)'B', (byte)'0', (byte)'D', (byte)'C', (byte)'8', (byte)'5', (byte)'B', (byte)'1', (byte)'1'
};

// Verify Method, Upgrade, Connection, version, key, etc..
public static bool CheckSupportedWebSocketRequest(string method, IEnumerable<KeyValuePair<string, string>> headers)
{
Expand Down Expand Up @@ -87,34 +96,34 @@ public static bool IsRequestKeyValid(string value)
{
return false;
}
try
{
byte[] data = Convert.FromBase64String(value);
return data.Length == 16;
}
catch (Exception)
{
return false;
}

Span<byte> temp = stackalloc byte[16];
var success = Convert.TryFromBase64String(value, temp, out var written);
return success && written == 16;
}

public static string CreateResponseKey(string requestKey)
{
// "The value of this header field is constructed by concatenating /key/, defined above in step 4
// in Section 4.2.2, with the string "258EAFA5- E914-47DA-95CA-C5AB0DC85B11", taking the SHA-1 hash of
// in Section 4.2.2, with the string "258EAFA5-E914-47DA-95CA-C5AB0DC85B11", taking the SHA-1 hash of
// this concatenated value to obtain a 20-byte value and base64-encoding"
// https://tools.ietf.org/html/rfc6455#section-4.2.2

if (requestKey == null)
{
throw new ArgumentNullException(nameof(requestKey));
}

using (var algorithm = SHA1.Create())
{
string merged = requestKey + "258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
byte[] mergedBytes = Encoding.UTF8.GetBytes(merged);
byte[] hashedBytes = algorithm.ComputeHash(mergedBytes);
// requestKey is already verified to be small (24 bytes) by 'IsRequestKeyValid()' and everything is 1:1 mapping to UTF8 bytes
// so this can be hardcoded to 60 bytes for the requestKey + static websocket string
Span<byte> mergedBytes = stackalloc byte[60];
Encoding.UTF8.GetBytes(requestKey, mergedBytes);
_encodedWebSocketKey.CopyTo(mergedBytes.Slice(24));
Copy link
Member

Choose a reason for hiding this comment

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

What if this is called with a requestKey that isn't 24 bytes? Should we check the return value of GetBytes?

Copy link
Member Author

Choose a reason for hiding this comment

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

"requestKey is already verified to be small (24 bytes) by 'IsRequestKeyValid()' and everything is 1:1 mapping to UTF8 bytes"


Span<byte> hashedBytes = stackalloc byte[20];
var success = algorithm.TryComputeHash(mergedBytes, hashedBytes, out var written);
if (!success || written != 20)
{
throw new InvalidOperationException("Could not compute the hash for the 'Sec-WebSocket-Accept' header.");
Copy link
Member

Choose a reason for hiding this comment

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

Is this the right behavior? a 500?

Copy link
Member

Choose a reason for hiding this comment

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

Probably. We don't know why it would fail and would have to debug it when it did.

Copy link
Member

Choose a reason for hiding this comment

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

This should be a 400 but we can wait until it happens 😄

}

return Convert.ToBase64String(hashedBytes);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>ASP.NET Core web socket middleware for use on top of opaque servers.</Description>
Expand All @@ -17,4 +17,8 @@
<Reference Include="Microsoft.Extensions.Options" />
</ItemGroup>

<ItemGroup>
<InternalsVisibleTo Include="Microsoft.AspNetCore.WebSockets.Tests" />
<InternalsVisibleTo Include="Microsoft.AspNetCore.WebSockets.MicroBenchmarks" />
</ItemGroup>
</Project>
3 changes: 1 addition & 2 deletions src/Middleware/WebSockets/src/WebSocketMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;
using Microsoft.Net.Http.Headers;
Expand Down Expand Up @@ -146,7 +145,7 @@ public async Task<WebSocket> AcceptAsync(WebSocketAcceptContext acceptContext)
}
}

string key = string.Join(", ", _context.Request.Headers[HeaderNames.SecWebSocketKey]);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was odd, nothing in the RFC says this is valid.
Should we check that there is only one value?

Copy link
Member

Choose a reason for hiding this comment

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

You're now using the implicit string converter that does the same thing here.

A multi-value header will fail the normal parse if there are multiple values anyways, don't worry about it.

Copy link
Member

Choose a reason for hiding this comment

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

Is it allocating right now even for a single value?

Copy link
Member

Choose a reason for hiding this comment

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

Is it allocating right now even for a single value?

No.

string key = _context.Request.Headers[HeaderNames.SecWebSocketKey];

HandshakeHelpers.GenerateResponseHeaders(key, subProtocol, _context.Response.Headers);

Expand Down
41 changes: 41 additions & 0 deletions src/Middleware/WebSockets/test/UnitTests/HandshakeTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Xunit;

namespace Microsoft.AspNetCore.WebSockets.Tests
{
public class HandshakeTests
{
[Fact]
public void CreatesCorrectResponseKey()
{
// Example taken from https://tools.ietf.org/html/rfc6455#section-1.3
var key = "dGhlIHNhbXBsZSBub25jZQ==";
var expectedResponse = "s3pPLMBiTxaQ9kYGzzhZRbK+xOo=";

var response = HandshakeHelpers.CreateResponseKey(key);

Assert.Equal(expectedResponse, response);
}

[Theory]
[InlineData("VUfWn1u2Ot0AICM6f+/8Zg==")]
public void AcceptsValidRequestKeys(string key)
{
Assert.True(HandshakeHelpers.IsRequestKeyValid(key));
}

[Theory]
// 17 bytes when decoded
[InlineData("dGhpcyBpcyAxNyBieXRlcy4=")]
// 15 bytes when decoded
[InlineData("dGhpcyBpcyAxNWJ5dGVz")]
[InlineData("")]
[InlineData("24 length not base64 str")]
public void RejectsInvalidRequestKeys(string key)
{
Assert.False(HandshakeHelpers.IsRequestKeyValid(key));
}
}
}
1 change: 1 addition & 0 deletions src/Middleware/perf/Microbenchmarks/AssemblyInfo.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[assembly: BenchmarkDotNet.Attributes.AspNetCoreBenchmark]
41 changes: 41 additions & 0 deletions src/Middleware/perf/Microbenchmarks/HandshakeBenchmark.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using BenchmarkDotNet.Attributes;

namespace Microsoft.AspNetCore.WebSockets.Microbenchmarks
{
public class HandshakeBenchmark
{
private string[] _requestKeys = {
"F8/qpj9RYr2/sIymdDvlmw==",
"PyQi8nyMkKnI7JKiAJ/IrA==",
"CUe0z8ItSBRtgJlPqP1+SQ==",
"w9vo1A9oM56M31qPQYKL6g==",
"+vqFGD9U04QOxKdWHrduTQ==",
"xsfuh2ZOm5O7zTzFPWJGUA==",
"TvmUzr4DgBLcDYX88kEAyw==",
"EZ5tcEOxWm7tF6adFXLSQg==",
"bkmoBhqwbbRzL8H9hvH1tQ==",
"EUwBrmmwivd5czsxz9eRzQ==",
};

[Benchmark(OperationsPerInvoke = 10)]
public void CreateResponseKey()
{
foreach (var key in _requestKeys)
{
HandshakeHelpers.CreateResponseKey(key);
}
}

[Benchmark(OperationsPerInvoke = 10)]
public void IsRequestKeyValid()
{
foreach (var key in _requestKeys)
{
HandshakeHelpers.IsRequestKeyValid(key);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
</PropertyGroup>

<ItemGroup>
<Reference Include="BenchmarkDotNet" />
<Reference Include="Microsoft.AspNetCore.BenchmarkRunner.Sources" />
<Reference Include="Microsoft.AspNetCore.WebSockets" />
</ItemGroup>
</Project>