Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix sending end stream in http2 web socket stream #73222

Merged
merged 7 commits into from
Aug 5, 2022
Merged

Fix sending end stream in http2 web socket stream #73222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
796617c
Fix sending end stream for websocket over H/2
Aug 2, 2022
d9b1858
Merge branch 'main' into fix-end-stream-ws-h2
Aug 2, 2022
c88e46b
feedback
Aug 2, 2022
92c8214
Update src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTe…
Aug 2, 2022
0d78ed8
default usessl in tests
Aug 3, 2022
1641793
Update src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.H…
Aug 5, 2022
d2d3ff8
disable new tests on browser because sockets not supported
Aug 5, 2022
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
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1599,7 +1599,7 @@ private async ValueTask<Http2Stream> SendHeadersAsync(HttpRequestMessage request
// Start the write. This serializes access to write to the connection, and ensures that HEADERS
// and CONTINUATION frames stay together, as they must do. We use the lock as well to ensure new
// streams are created and started in order.
await PerformWriteAsync(totalSize, (thisRef: this, http2Stream, headerBytes, endStream: (request.Content == null), mustFlush), static (s, writeBuffer) =>
await PerformWriteAsync(totalSize, (thisRef: this, http2Stream, headerBytes, endStream: (request.Content == null && !http2Stream.ConnectProtocolEstablished), mustFlush), static (s, writeBuffer) =>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm doing something wrong, or misunderstanding the change, but if I'm reading this correctly and testing the new feature correctly, this check does nothing because ConnectProtocolEstablished is only set after the request has already been sent and a response with a 200 was received.

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs

Lines 640 to 643 in 08a11e4

if (statusCode == 200 && _response.RequestMessage!.IsWebSocketH2Request())
{
ConnectProtocolEstablished = true;
}

Which means every time an HTTP2 WebSocket request is made it will still send the END_STREAM flag.

Did you test this change with Kestrel before merging? Am I holding the feature wrong?

cc @karelz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I will double check. I tested it with Kestrel but I probably miss to check state on the server side. Using receive before send works so I thought that the end stream flag causes an issue only on established connection. Also connect without end stream is hanging, I should try different approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I have to switch on flush correctly for both connect and send

{
if (NetEventSource.Log.IsEnabled()) s.thisRef.Trace(s.http2Stream.StreamId, $"Started writing. Total header bytes={s.headerBytes.Length}");

Expand Down
7 changes: 4 additions & 3 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ private sealed class Http2Stream : IValueTaskSource, IHttpStreamHeadersHandler,
private StreamCompletionState _responseCompletionState;
private ResponseProtocolState _responseProtocolState;
private bool _responseHeadersReceived;
private bool _webSocketEstablished;

// If this is not null, then we have received a reset from the server
// (i.e. RST_STREAM or general IO error processing the connection)
Expand Down Expand Up @@ -158,6 +157,8 @@ public void Initialize(int streamId, int initialWindowSize)

public Http2Connection Connection => _connection;

public bool ConnectProtocolEstablished { get; private set; }

public HttpResponseMessage GetAndClearResponse()
{
// Once SendAsync completes, the Http2Stream should no longer hold onto the response message.
Expand Down Expand Up @@ -638,7 +639,7 @@ private void OnStatus(int statusCode)
{
if (statusCode == 200 && _response.RequestMessage!.IsWebSocketH2Request())
{
_webSocketEstablished = true;
ConnectProtocolEstablished = true;
}

_responseProtocolState = ResponseProtocolState.ExpectingHeaders;
Expand Down Expand Up @@ -1041,7 +1042,7 @@ public async Task ReadResponseHeadersAsync(CancellationToken cancellationToken)
MoveTrailersToResponseMessage(_response);
responseContent.SetStream(EmptyReadStream.Instance);
}
else if (_webSocketEstablished)
else if (ConnectProtocolEstablished)
{
responseContent.SetStream(new Http2ReadWriteStream(this));
}
Expand Down
89 changes: 80 additions & 9 deletions src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.Http2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,56 @@ public class ConnectTest_Http2 : ClientWebSocketTestBase
{
public ConnectTest_Http2(ITestOutputHelper output) : base(output) { }

[Theory]
[InlineData(false)]
[InlineData(true)]
[SkipOnPlatform(TestPlatforms.Browser, "System.Net.Sockets is not supported on this platform")]
public async Task ConnectAsync_VersionNotSupported_NoSsl_Throws(bool useHandler)
{
await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
{
using (var cws = new ClientWebSocket())
using (var cts = new CancellationTokenSource(TimeOutMilliseconds))
{
cws.Options.HttpVersion = HttpVersion.Version20;
cws.Options.HttpVersionPolicy = Http.HttpVersionPolicy.RequestVersionExact;
Task t;
if (useHandler)
{
var handler = new SocketsHttpHandler();
t = cws.ConnectAsync(uri, new HttpMessageInvoker(handler), cts.Token);
}
else
{
t = cws.ConnectAsync(uri, cts.Token);
}
var ex = await Assert.ThrowsAnyAsync<WebSocketException>(() => t);
Assert.IsType<HttpRequestException>(ex.InnerException);
Assert.True(ex.InnerException.Data.Contains("SETTINGS_ENABLE_CONNECT_PROTOCOL"));
}
},
async server =>
{
Http2LoopbackConnection connection = await server.EstablishConnectionAsync(new SettingsEntry { SettingId = SettingId.EnableConnect, Value = 0 });
}, new Http2Options() { WebSocketEndpoint = true, UseSsl = false }
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
);
}

[Fact]
[SkipOnPlatform(TestPlatforms.Browser, "Self-signed certificates are not supported on browser")]
public async Task ConnectAsync_VersionNotSupported_Throws()
public async Task ConnectAsync_VersionNotSupported_WithSsl_Throws()
{
await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
{
using (var clientSocket = new ClientWebSocket())
using (var cws = new ClientWebSocket())
using (var cts = new CancellationTokenSource(TimeOutMilliseconds))
{
clientSocket.Options.HttpVersion = HttpVersion.Version20;
clientSocket.Options.HttpVersionPolicy = Http.HttpVersionPolicy.RequestVersionExact;
using var handler = CreateSocketsHttpHandler(allowAllCertificates: true);
Task t = clientSocket.ConnectAsync(uri, new HttpMessageInvoker(handler), cts.Token);
cws.Options.HttpVersion = HttpVersion.Version20;
cws.Options.HttpVersionPolicy = Http.HttpVersionPolicy.RequestVersionExact;
Task t;
var handler = CreateSocketsHttpHandler(allowAllCertificates: true);
t = cws.ConnectAsync(uri, new HttpMessageInvoker(handler), cts.Token);

var ex = await Assert.ThrowsAnyAsync<WebSocketException>(() => t);
Assert.IsType<HttpRequestException>(ex.InnerException);
Assert.True(ex.InnerException.Data.Contains("SETTINGS_ENABLE_CONNECT_PROTOCOL"));
Expand All @@ -44,9 +81,42 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
);
}

[Theory]
[InlineData(false)]
[InlineData(true)]
[SkipOnPlatform(TestPlatforms.Browser, "System.Net.Sockets is not supported on this platform")]
public async Task ConnectAsync_VersionSupported_NoSsl_Success(bool useHandler)
{
await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
{
using (var cws = new ClientWebSocket())
using (var cts = new CancellationTokenSource(TimeOutMilliseconds))
{
cws.Options.HttpVersion = HttpVersion.Version20;
cws.Options.HttpVersionPolicy = Http.HttpVersionPolicy.RequestVersionExact;
if (useHandler)
{
var handler = new SocketsHttpHandler();
await cws.ConnectAsync(uri, new HttpMessageInvoker(handler), cts.Token);
}
else
{
await cws.ConnectAsync(uri, cts.Token);
}
}
},
async server =>
{
Http2LoopbackConnection connection = await server.EstablishConnectionAsync(new SettingsEntry { SettingId = SettingId.EnableConnect, Value = 1 });
(int streamId, HttpRequestData requestData) = await connection.ReadAndParseRequestHeaderAsync(readBody: false);
await connection.SendResponseHeadersAsync(streamId, endStream: false, HttpStatusCode.OK);
}, new Http2Options() { WebSocketEndpoint = true, UseSsl = false }
);
}

[Fact]
[SkipOnPlatform(TestPlatforms.Browser, "Self-signed certificates are not supported on browser")]
public async Task ConnectAsync_VersionSupported_Success()
public async Task ConnectAsync_VersionSupported_WithSsl_Success()
{
await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
{
Expand All @@ -55,14 +125,15 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
{
cws.Options.HttpVersion = HttpVersion.Version20;
cws.Options.HttpVersionPolicy = Http.HttpVersionPolicy.RequestVersionExact;
using var handler = CreateSocketsHttpHandler(allowAllCertificates: true);

var handler = CreateSocketsHttpHandler(allowAllCertificates: true);
await cws.ConnectAsync(uri, new HttpMessageInvoker(handler), cts.Token);
}
},
async server =>
{
Http2LoopbackConnection connection = await server.EstablishConnectionAsync(new SettingsEntry { SettingId = SettingId.EnableConnect, Value = 1 });
(int streamId, HttpRequestData requestData) = await connection.ReadAndParseRequestHeaderAsync(readBody : false);
(int streamId, HttpRequestData requestData) = await connection.ReadAndParseRequestHeaderAsync(readBody: false);
await connection.SendResponseHeadersAsync(streamId, endStream: false, HttpStatusCode.OK);
}, new Http2Options() { WebSocketEndpoint = true }
);
Expand Down
110 changes: 110 additions & 0 deletions src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.Http2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Linq;
using System.Net.Http;
using System.Net.Sockets;
using System.Net.Test.Common;
using System.Threading;
using System.Threading.Tasks;

using Xunit;
using Xunit.Abstractions;

using static System.Net.Http.Functional.Tests.TestHelper;

namespace System.Net.WebSockets.Client.Tests
{
public class SendReceiveTest_Http2 : ClientWebSocketTestBase
{
public SendReceiveTest_Http2(ITestOutputHelper output) : base(output) { }

[Theory]
[InlineData(false)]
[InlineData(true)]
[SkipOnPlatform(TestPlatforms.Browser, "System.Net.Sockets is not supported on this platform")]
public async Task ReceiveNoThrowAfterSend_NoSsl(bool useHandler)
{
var serverMessage = new byte[] { 4, 5, 6 };
await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
{
using (var cws = new ClientWebSocket())
using (var cts = new CancellationTokenSource(TimeOutMilliseconds))
{
cws.Options.HttpVersion = HttpVersion.Version20;
cws.Options.HttpVersionPolicy = Http.HttpVersionPolicy.RequestVersionExact;
if (useHandler)
{
var handler = new SocketsHttpHandler();
await cws.ConnectAsync(uri, new HttpMessageInvoker(handler), cts.Token);
}
else
{
await cws.ConnectAsync(uri, cts.Token);
}

await cws.SendAsync(new byte[] { 2, 3, 4 }, WebSocketMessageType.Binary, true, cts.Token);

var readBuffer = new byte[serverMessage.Length];
await cws.ReceiveAsync(readBuffer, cts.Token);
Assert.Equal(serverMessage, readBuffer);
}
},
async server =>
{
Http2LoopbackConnection connection = await server.EstablishConnectionAsync(new SettingsEntry { SettingId = SettingId.EnableConnect, Value = 1 });
(int streamId, HttpRequestData requestData) = await connection.ReadAndParseRequestHeaderAsync(readBody: false);
// send status 200 OK to establish websocket
await connection.SendResponseHeadersAsync(streamId, endStream: false).ConfigureAwait(false);

// send reply
byte binaryMessageType = 2;
var prefix = new byte[] { binaryMessageType, (byte)serverMessage.Length };
byte[] constructMessage = prefix.Concat(serverMessage).ToArray();
await connection.SendResponseDataAsync(streamId, constructMessage, endStream: false);

}, new Http2Options() { WebSocketEndpoint = true, UseSsl = false }
);
}

[Fact]
[SkipOnPlatform(TestPlatforms.Browser, "Self-signed certificates are not supported on browser")]
public async Task ReceiveNoThrowAfterSend_WithSsl()
{
var serverMessage = new byte[] { 4, 5, 6 };
await Http2LoopbackServer.CreateClientAndServerAsync(async uri =>
{
using (var cws = new ClientWebSocket())
using (var cts = new CancellationTokenSource(TimeOutMilliseconds))
{
cws.Options.HttpVersion = HttpVersion.Version20;
cws.Options.HttpVersionPolicy = Http.HttpVersionPolicy.RequestVersionExact;

var handler = CreateSocketsHttpHandler(allowAllCertificates: true);
await cws.ConnectAsync(uri, new HttpMessageInvoker(handler), cts.Token);

await cws.SendAsync(new byte[] { 2, 3, 4 }, WebSocketMessageType.Binary, true, cts.Token);

var readBuffer = new byte[serverMessage.Length];
await cws.ReceiveAsync(readBuffer, cts.Token);
Assert.Equal(serverMessage, readBuffer);
}
},
async server =>
{
Http2LoopbackConnection connection = await server.EstablishConnectionAsync(new SettingsEntry { SettingId = SettingId.EnableConnect, Value = 1 });
(int streamId, HttpRequestData requestData) = await connection.ReadAndParseRequestHeaderAsync(readBody: false);
// send status 200 OK to establish websocket
await connection.SendResponseHeadersAsync(streamId, endStream: false).ConfigureAwait(false);

// send reply
byte binaryMessageType = 2;
var prefix = new byte[] { binaryMessageType, (byte)serverMessage.Length };
byte[] constructMessage = prefix.Concat(serverMessage).ToArray();
await connection.SendResponseDataAsync(streamId, constructMessage, endStream: false);

}, new Http2Options() { WebSocketEndpoint = true }
);
}
}
}
1 change: 1 addition & 0 deletions src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
<Compile Include="LoopbackHelper.cs" />
<Compile Include="ResourceHelper.cs" />
<Compile Include="SendReceiveTest.cs" />
<Compile Include="SendReceiveTest.Http2.cs" />
<Compile Include="WebSocketData.cs" />
<Compile Include="WebSocketHelper.cs" />
<Compile Include="DeflateTests.cs" />
Expand Down