-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Websocket handshake perf #12386
Changes from all commits
d9fa1c9
df8bbf6
ac10857
cf0c849
bb171a7
453abd8
6c4a760
3507ce9
9ec7ce5
00d65d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
{ | ||
|
@@ -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)); | ||
|
||
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."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the right behavior? a 500? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -146,7 +145,7 @@ public async Task<WebSocket> AcceptAsync(WebSocketAcceptContext acceptContext) | |
} | ||
} | ||
|
||
string key = string.Join(", ", _context.Request.Headers[HeaderNames.SecWebSocketKey]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was odd, nothing in the RFC says this is valid. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it allocating right now even for a single value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
string key = _context.Request.Headers[HeaderNames.SecWebSocketKey]; | ||
|
||
HandshakeHelpers.GenerateResponseHeaders(key, subProtocol, _context.Response.Headers); | ||
|
||
|
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)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
[assembly: BenchmarkDotNet.Attributes.AspNetCoreBenchmark] |
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"