Skip to content

don't dispose client control stream before closing connection #57223

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

Merged
geoffkizer merged 5 commits into from
Aug 17, 2021
Merged

don't dispose client control stream before closing connection #57223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79f755d
don't dispose client control stream before closing connection
Aug 11, 2021
46fd7ef
Merge branch 'main' into dontclosecontrolstream
geoffkizer Aug 11, 2021
c9c4c82
fix spacing issue cause by github merge
Aug 12, 2021
ae3bdbe
Don't dispose the loopback connection in Http3LoopbackConnection.Dispose
Aug 17, 2021
8331cda
remove _closed flag and related assert
Aug 17, 2021
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
80 changes: 61 additions & 19 deletions src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading.Tasks;
using System.Linq;
using System.Net.Http.Functional.Tests;
using Xunit;

namespace System.Net.Test.Common
{
Expand All @@ -32,10 +33,13 @@ internal sealed class Http3LoopbackConnection : GenericLoopbackConnection
public const long H3_VERSION_FALLBACK = 0x110;

private readonly QuicConnection _connection;

// This is specifically request streams, not control streams
private readonly Dictionary<int, Http3LoopbackStream> _openStreams = new Dictionary<int, Http3LoopbackStream>();
private Http3LoopbackStream _controlStream; // Our outbound control stream
private Http3LoopbackStream _currentStream;
private bool _closed;

private Http3LoopbackStream _inboundControlStream; // Inbound control stream from client
private Http3LoopbackStream _outboundControlStream; // Our outbound control stream

public Http3LoopbackConnection(QuicConnection connection)
{
Expand All @@ -44,23 +48,30 @@ public Http3LoopbackConnection(QuicConnection connection)

public override void Dispose()
{
// Close any remaining request streams (but NOT control streams, as these should not be closed while the connection is open)
foreach (Http3LoopbackStream stream in _openStreams.Values)
{
stream.Dispose();
}

if (!_closed)
{
// CloseAsync(H3_INTERNAL_ERROR).GetAwaiter().GetResult();
}

//_connection.Dispose();
// We don't dispose the connection currently, because this causes races when the server connection is closed before
// the client has received and handled all response data.
// See discussion in https://github.com/dotnet/runtime/pull/57223#discussion_r687447832
#if false
// Dispose the connection
// If we already waited for graceful shutdown from the client, then the connection is already closed and this will simply release the handle.
// If not, then this will silently abort the connection.
_connection.Dispose();

@ManickaP ManickaP Aug 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will this cause us to drop connections prematurely again? I know that the close/dispose were commented out, because the server task would sometimes finish before the client. It would close the connection from the server side and then the client response reading would throw due to a closed stream.

@geoffkizer geoffkizer Aug 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

The original problem was with code that would use CreateClientAndServer. The problem here was that we would spawn a task for the client and another for the server, then wait for them both. And the server task would dispose the server connection when it completed. This could result in losing data. The fix was to have the server wait for graceful shutdown in these cases.

Most code that doesn't use CreateClientAndServer should not have this problem.

Note though that if code does have this problem, then it already has it because even though we didn't dispose the connection here previously, it was still collectible at this point, meaning that we would likely see occasional failures here. So actively disposing the connection here is probably better since it will be more likely to fail consistently.

@ManickaP ManickaP Aug 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that we might revisit some of the H/3 tests and see if this might be a problem there. For example:

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs

Line 35 in c8dc49e

public async Task ClientSettingsReceived_Success(int headerSizeLimit)

It's disabled now so we wouldn't see any problem, but I think it might suffer from this problem. I assume that adding WaitForClientDisconnectAsync at the end of the server task should mitigate this, am I right?

@geoffkizer geoffkizer Aug 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, there could be a problem here, unfortunately.

I assume that adding WaitForClientDisconnectAsync at the end of the server task should mitigate this, am I right?

Yes, I believe that would handle it. That said, I think we should revisit how this works generally, as it is slightly different for every version of HTTP, which seems bad. That's not something to tackle for 6.0 though.

I think what we should do here is just revert the change here that disposes the connection, to minimize behavioral change in the loopback server. I'll update and post a new version of the PR.


// Dispose control streams so that we release their handles too.
_inboundControlStream?.Dispose();

@ManickaP ManickaP Aug 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any reason for this order of disposes? I guess it doesn't matter since we do the stream counting in the connection, but I thought the goal was to do:

  • connection.Close
  • controlStream.Dispose
  • connection.Dispose

Anyway, I don't mind this, I'm just curious if there's some additional benefit.

@geoffkizer geoffkizer Aug 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As long as we close the connection first, it shouldn't matter.

That said, in this path we may or may not have closed the connection (as per comment above). If we haven't, then it does matter because if we dispose the stream before the connection, this will abort the stream because the connection is still open. We don't want that.

_outboundControlStream?.Dispose();
#endif
}

public async Task CloseAsync(long errorCode)
{
await _connection.CloseAsync(errorCode).ConfigureAwait(false);
_closed = true;
}

public Http3LoopbackStream OpenUnidirectionalStream()
Expand Down Expand Up @@ -96,23 +107,50 @@ public async Task<Http3LoopbackStream> AcceptStreamAsync()
QuicStream quicStream = await _connection.AcceptStreamAsync().ConfigureAwait(false);
var stream = new Http3LoopbackStream(quicStream);

_openStreams.Add(checked((int)quicStream.StreamId), stream);
_currentStream = stream;
if (quicStream.CanWrite)
{
_openStreams.Add(checked((int)quicStream.StreamId), stream);
_currentStream = stream;
}

return stream;
}

private async Task HandleControlStreamAsync(Http3LoopbackStream controlStream)
{
if (_inboundControlStream is not null)
{
throw new Exception("Received second control stream from client???");
}

long? streamType = await controlStream.ReadIntegerAsync();
Assert.Equal(Http3LoopbackStream.ControlStream, streamType);

List<(long settingId, long settingValue)> settings = await controlStream.ReadSettingsAsync();
(long settingId, long settingValue) = Assert.Single(settings);

Assert.Equal(Http3LoopbackStream.MaxHeaderListSize, settingId);

_inboundControlStream = controlStream;
}

// This will automatically handle the control stream, including validating its contents
public async Task<Http3LoopbackStream> AcceptRequestStreamAsync()
{
Http3LoopbackStream stream;

do
while (true)
{
stream = await AcceptStreamAsync().ConfigureAwait(false);
}
while (!stream.CanWrite); // skip control stream.

return stream;
if (stream.CanWrite)
{
return stream;
}

// Must be the control stream
await HandleControlStreamAsync(stream);
}
}

public async Task<(Http3LoopbackStream clientControlStream, Http3LoopbackStream requestStream)> AcceptControlAndRequestStreamAsync()
Expand Down Expand Up @@ -141,9 +179,9 @@ public async Task<Http3LoopbackStream> AcceptRequestStreamAsync()

public async Task EstablishControlStreamAsync()
{
_controlStream = OpenUnidirectionalStream();
await _controlStream.SendUnidirectionalStreamTypeAsync(Http3LoopbackStream.ControlStream);
await _controlStream.SendSettingsFrameAsync();
_outboundControlStream = OpenUnidirectionalStream();
await _outboundControlStream.SendUnidirectionalStreamTypeAsync(Http3LoopbackStream.ControlStream);
await _outboundControlStream.SendSettingsFrameAsync();
}

public override async Task<byte[]> ReadRequestBodyAsync()
Expand Down Expand Up @@ -185,7 +223,7 @@ public override async Task<HttpRequestData> HandleRequestAsync(HttpStatusCode st

// We are about to close the connection, after we send the response.
// So, send a GOAWAY frame now so the client won't inadvertantly try to reuse the connection.
await _controlStream.SendGoAwayFrameAsync(stream.StreamId + 4);
await _outboundControlStream.SendGoAwayFrameAsync(stream.StreamId + 4);

await stream.SendResponseAsync(statusCode, headers, content).ConfigureAwait(false);

Expand Down Expand Up @@ -221,6 +259,10 @@ public async Task WaitForClientDisconnectAsync(bool refuseNewRequests = true)
}
}

// The client's control stream should throw QuicConnectionAbortedException, indicating that it was
// aborted because the connection was closed (and was not explicitly closed or aborted prior to the connection being closed)
await Assert.ThrowsAsync<QuicConnectionAbortedException>(async () => await _inboundControlStream.ReadFrameAsync());

await CloseAsync(H3_NO_ERROR);
}

Expand Down
13 changes: 7 additions & 6 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,6 @@ private void CheckForShutdown()
return;
}

if (_clientControl != null)
{
_clientControl.Dispose();
_clientControl = null;
}

if (_connection != null)
{
// Close the QuicConnection in the background.
Expand Down Expand Up @@ -146,6 +140,13 @@ private void CheckForShutdown()
{
Trace($"{nameof(QuicConnection)} failed to dispose: {ex}");
}

if (_clientControl != null)
{
_clientControl.Dispose();
_clientControl = null;
}

}, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
}
}
Expand Down
14 changes: 14 additions & 0 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,20 @@ internal sealed class MockConnection : QuicConnectionProvider
internal PeerStreamLimit? LocalStreamLimit => _isClient ? _state?._clientStreamLimit : _state?._serverStreamLimit;
internal PeerStreamLimit? RemoteStreamLimit => _isClient ? _state?._serverStreamLimit : _state?._clientStreamLimit;

internal long? ConnectionError
{
get
{
long? errorCode = _isClient ? _state?._serverErrorCode : _state?._clientErrorCode;
if (errorCode == -1)
{
errorCode = null;
}

return errorCode;
}
}

internal override X509Certificate? RemoteCertificate => null;

// Constructor for outbound connections
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ internal override async ValueTask<int> ReadAsync(Memory<byte> buffer, Cancellati
int bytesRead = await streamBuffer.ReadAsync(buffer, cancellationToken).ConfigureAwait(false);
if (bytesRead == 0)
{
if (_connection.ConnectionError is long connectonError)
{
throw new QuicConnectionAbortedException(connectonError);
}

long errorCode = _isInitiator ? _streamState._inboundReadErrorCode : _streamState._outboundReadErrorCode;
if (errorCode != 0)
{
Expand Down Expand Up @@ -135,6 +140,11 @@ internal override async ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, bool e
throw new NotSupportedException();
}

if (_connection.ConnectionError is long connectonError)
{
throw new QuicConnectionAbortedException(connectonError);
}

long errorCode = _isInitiator ? _streamState._inboundWriteErrorCode : _streamState._outboundWriteErrorCode;
if (errorCode != 0)
{
Expand Down