Skip to content

Commit

Permalink
Do not directly abort connection on GOAWAY (dotnet/corefx#39036)
Browse files Browse the repository at this point in the history 
* Do not directly abort connection on GOAWAY

* Address feedback and add disabled GOAWAY tests

* remove redundant frame ignoring logic


Commit migrated from dotnet/corefx@cc6435b
  • Loading branch information
@krwq
krwq authored Jul 1, 2019
1 parent 210eeeb commit 61de06b
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 12 deletions.
47 changes: 35 additions & 12 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ internal sealed partial class Http2Connection : HttpConnectionBase, IDisposable
private int _pendingWriters;

private bool _disposed;
// anything else than -1 means that connection is being terminated
// in which case _lastValidStreamId is the maximum StreamId which can be processed
private int _lastValidStreamId = -1;
private Exception _abortException;

// If an in-progress write is canceled we need to be able to immediately
Expand Down Expand Up @@ -1278,17 +1281,26 @@ public bool IsExpired(long nowTicks,

private void AbortStreams(int lastValidStream, Exception abortException)
{
bool shouldInvalidate = false;
Debug.Assert(lastValidStream >= 0);
bool isAlreadyInvalidated = _disposed;

lock (SyncObject)
{
if (NetEventSource.IsEnabled) Trace($"{nameof(lastValidStream)}={lastValidStream}, {nameof(abortException)}={lastValidStream}={abortException}");

if (!_disposed)
if (_lastValidStreamId == -1)
{
shouldInvalidate = true;
_disposed = true;
_lastValidStreamId = lastValidStream;
}
else
{
// We have already received GOAWAY before
// In this case the smaller valid stream is used
_lastValidStreamId = Math.Min(_lastValidStreamId, lastValidStream);
isAlreadyInvalidated = true;
}

if (NetEventSource.IsEnabled) Trace($"{nameof(lastValidStream)}={lastValidStream}, {nameof(abortException)}={lastValidStream}={abortException}, {nameof(_lastValidStreamId)}={_lastValidStreamId}");

bool hasAnyActiveStream = false;
foreach (KeyValuePair<int, Http2Stream> kvp in _httpStreams)
{
int streamId = kvp.Key;
Expand All @@ -1300,16 +1312,23 @@ private void AbortStreams(int lastValidStream, Exception abortException)

_httpStreams.Remove(kvp.Value.StreamId);
}
else if (NetEventSource.IsEnabled)
else
{
Trace($"Found {nameof(streamId)} {streamId} <= {lastValidStream}.");
if (NetEventSource.IsEnabled) Trace($"Found {nameof(streamId)} {streamId} <= {lastValidStream}.");

hasAnyActiveStream = true;
}
}

if (!hasAnyActiveStream)
{
_disposed = true;
}

CheckForShutdown();
}

if (shouldInvalidate)
if (!isAlreadyInvalidated)
{
// Invalidate outside of lock to avoid race with HttpPool Dispose()
// We should not try to grab pool lock while holding connection lock as on disposing pool,
Expand All @@ -1320,14 +1339,18 @@ private void AbortStreams(int lastValidStream, Exception abortException)

private void CheckForShutdown()
{
Debug.Assert(_disposed);
Debug.Assert(_disposed || _lastValidStreamId != -1);
Debug.Assert(Monitor.IsEntered(SyncObject));

// Check if dictionary has become empty
if (_httpStreams.Count != 0)
{
return;
}
else
{
_disposed = true;
}

// Do shutdown.
_stream.Close();
Expand Down Expand Up @@ -1560,7 +1583,7 @@ private Http2Stream AddStream(HttpRequestMessage request)
{
lock (SyncObject)
{
if (_nextStream == MaxStreamId || _disposed)
if (_nextStream == MaxStreamId || _disposed || _lastValidStreamId != -1)
{
if (_abortException != null)
{
Expand Down Expand Up @@ -1613,7 +1636,7 @@ private void RemoveStream(Http2Stream http2Stream)
_idleSinceTickCount = Environment.TickCount64;
}

if (_disposed)
if (_disposed || _lastValidStreamId != -1)
{
CheckForShutdown();
}
Expand Down
114 changes: 114 additions & 0 deletions src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,120 @@ public async Task GoAwayFrame_NewRequest_NewConnection()
}
}

[ConditionalFact(nameof(SupportsAlpn))]
public async Task GoAwayFrame_ServerDisconnect_AbortStreams()
{
using (var server = Http2LoopbackServer.CreateServer())
using (HttpClient client = CreateHttpClient())
{
Task<HttpResponseMessage> sendTask = client.GetAsync(server.Address);
Http2LoopbackConnection connection = await server.EstablishConnectionAsync();
int streamId = await connection.ReadRequestHeaderAsync();

await connection.SendGoAway(streamId);

// wait until GOAWAY received
await connection.PingPong();

// Shutdown the connection unexpectedly
await connection.WaitForConnectionShutdownAsync();

// Expect client to detect that server has disconnected and throw an exception
await Assert.ThrowsAnyAsync<HttpRequestException>(() =>
new Task[]
{
sendTask
}.WhenAllOrAnyFailed(TestHelper.PassingTestTimeoutMilliseconds));
}
}

[ConditionalFact(nameof(SupportsAlpn))]
[ActiveIssue(39013)]
public async Task GoAwayFrame_UnprocessedStreamFirstRequestFinishedFirst_RequestRestarted()
{
// This test case is similar to GoAwayFrame_UnprocessedStreamFirstRequestWaitsUntilSecondFinishes_RequestRestarted
// but is easier: we close first connection before we expect retry to happen

using (var server = Http2LoopbackServer.CreateServer())
using (HttpClient client = CreateHttpClient())
{
server.AllowMultipleConnections = true;

Task<HttpResponseMessage> sendTask1 = client.GetAsync(server.Address);
Http2LoopbackConnection connection1 = await server.EstablishConnectionAsync();
int streamId1 = await connection1.ReadRequestHeaderAsync();

Task<HttpResponseMessage> sendTask2 = client.GetAsync(server.Address);
int streamId2 = await connection1.ReadRequestHeaderAsync();

// Client should try to restart on new connection when GOAWAY is sent
Task<Http2LoopbackConnection> waitForSecondConnection = server.EstablishConnectionAsync();

await connection1.SendGoAway(streamId1);

// Wait until GOAWAY received
await connection1.PingPong();

// Complete first request
await connection1.SendDefaultResponseAsync(streamId1);
HttpResponseMessage response1 = await sendTask1;
Assert.Equal(HttpStatusCode.OK, response1.StatusCode);
await connection1.WaitForConnectionShutdownAsync();

// Client should retry connection
Http2LoopbackConnection connection2 = await waitForSecondConnection;

// Complete second request
int streamId3 = await connection2.ReadRequestHeaderAsync();
await connection2.SendDefaultResponseAsync(streamId3);
HttpResponseMessage response2 = await sendTask2;
Assert.Equal(HttpStatusCode.OK, response2.StatusCode);
await connection2.WaitForConnectionShutdownAsync();
}
}

[ConditionalFact(nameof(SupportsAlpn))]
[ActiveIssue(39013)]
public async Task GoAwayFrame_UnprocessedStreamFirstRequestWaitsUntilSecondFinishes_RequestRestarted()
{
using (var server = Http2LoopbackServer.CreateServer())
using (HttpClient client = CreateHttpClient())
{
server.AllowMultipleConnections = true;

Task<HttpResponseMessage> sendTask1 = client.GetAsync(server.Address);
Http2LoopbackConnection connection1 = await server.EstablishConnectionAsync();
int streamId1 = await connection1.ReadRequestHeaderAsync();

Task<HttpResponseMessage> sendTask2 = client.GetAsync(server.Address);
int streamId2 = await connection1.ReadRequestHeaderAsync();

// Client should try to restart on new connection when GOAWAY is sent
Task<Http2LoopbackConnection> waitForSecondConnection = server.EstablishConnectionAsync();

await connection1.SendGoAway(streamId1);

// Wait until GOAWAY received
await connection1.PingPong();

// Client should retry connection
Http2LoopbackConnection connection2 = await waitForSecondConnection;

// Complete second request
int streamId3 = await connection2.ReadRequestHeaderAsync();
await connection2.SendDefaultResponseAsync(streamId3);
HttpResponseMessage response2 = await sendTask2;
Assert.Equal(HttpStatusCode.OK, response2.StatusCode);
await connection2.WaitForConnectionShutdownAsync();

// Complete first request
await connection1.SendDefaultResponseAsync(streamId1);
HttpResponseMessage response1 = await sendTask1;
Assert.Equal(HttpStatusCode.OK, response1.StatusCode);
await connection1.WaitForConnectionShutdownAsync();
}
}

[ConditionalFact(nameof(SupportsAlpn))]
public async Task DataFrame_TooLong_ConnectionError()
{
Expand Down

0 comments on commit 61de06b

Please sign in to comment.