diff --git a/src/libraries/System.Net.Http/src/Resources/Strings.resx b/src/libraries/System.Net.Http/src/Resources/Strings.resx index 8af485a17fee14..41314d822c3792 100644 --- a/src/libraries/System.Net.Http/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http/src/Resources/Strings.resx @@ -423,6 +423,9 @@ The initial HTTP/2 stream window size must be between {0} and {1}. + + The HTTP/2 connection was aborted because the size of the frame queue exceeded an internal limit. + This method is not implemented by this class. diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs index cc53c81c94e7d6..e1adf9e7e58cf5 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs @@ -75,6 +75,10 @@ internal sealed partial class Http2Connection : HttpConnectionBase private Http2ProtocolErrorCode? _goAwayErrorCode; + // Cap number of untransmitted PING and SETTING ACKs and PING requests + private const int MaxQueuedFireAndForgetFrames = 1000; + private int _queuedFireAndForgetFrames; + private const int MaxStreamId = int.MaxValue; // Temporary workaround for request burst handling on connection start. @@ -926,7 +930,7 @@ private void ProcessSettingsFrame(FrameHeader frameHeader, bool initialFrame = f // Send acknowledgement // Don't wait for completion, which could happen asynchronously. - LogExceptions(SendSettingsAckAsync()); + QueueSettingsAck(); } } @@ -1007,7 +1011,7 @@ private void ProcessPingFrame(FrameHeader frameHeader) } else { - LogExceptions(SendPingAsync(pingContentLong, isAck: true)); + QueuePing(pingContentLong, isAck: true); } _incomingBuffer.Discard(frameHeader.PayloadLength); } @@ -1288,20 +1292,35 @@ private async Task ProcessOutgoingFramesAsync() } } - private Task SendSettingsAckAsync() => - PerformWriteAsync(FrameHeader.Size, this, static (thisRef, writeBuffer) => + private void QueueSettingsAck() + { + if (!TryIncrementQueuedFireAndForgetFrames()) + { + return; + } + + LogExceptions(PerformWriteAsync(FrameHeader.Size, this, static (thisRef, writeBuffer) => { if (NetEventSource.Log.IsEnabled()) thisRef.Trace("Started writing."); FrameHeader.WriteTo(writeBuffer.Span, 0, FrameType.Settings, FrameFlags.Ack, streamId: 0); + thisRef.DecrementQueuedFireAndForgetFrames(); + return true; - }); + })); + } /// The 8-byte ping content to send, read as a big-endian integer. /// Determine whether the frame is ping or ping ack. - private Task SendPingAsync(long pingContent, bool isAck = false) => - PerformWriteAsync(FrameHeader.Size + FrameHeader.PingLength, (thisRef: this, pingContent, isAck), static (state, writeBuffer) => + private void QueuePing(long pingContent, bool isAck = false) + { + if (!TryIncrementQueuedFireAndForgetFrames()) + { + return; + } + + LogExceptions(PerformWriteAsync(FrameHeader.Size + FrameHeader.PingLength, (thisRef: this, pingContent, isAck), static (state, writeBuffer) => { if (NetEventSource.Log.IsEnabled()) state.thisRef.Trace($"Started writing. {nameof(pingContent)}={state.pingContent}"); @@ -1311,8 +1330,11 @@ private Task SendPingAsync(long pingContent, bool isAck = false) => FrameHeader.WriteTo(span, FrameHeader.PingLength, FrameType.Ping, state.isAck ? FrameFlags.Ack : FrameFlags.None, streamId: 0); BinaryPrimitives.WriteInt64BigEndian(span.Slice(FrameHeader.Size), state.pingContent); + state.thisRef.DecrementQueuedFireAndForgetFrames(); + return true; - }); + })); + } private Task SendRstStreamAsync(int streamId, Http2ProtocolErrorCode errorCode) => PerformWriteAsync(FrameHeader.Size + FrameHeader.RstStreamLength, (thisRef: this, streamId, errorCode), static (s, writeBuffer) => @@ -1815,6 +1837,28 @@ private bool ForceSendConnectionWindowUpdate() return true; } + private bool TryIncrementQueuedFireAndForgetFrames() + { + if (Interlocked.Increment(ref _queuedFireAndForgetFrames) > MaxQueuedFireAndForgetFrames) + { + if (NetEventSource.Log.IsEnabled()) this.Trace("Number of untransmitted PING and SETTING frames exceeded limit."); + + // Close connection when there is too much outstanding frames + var ex = new HttpIOException(HttpRequestError.Unknown, SR.net_http_http2_frame_limit_exceeded); + Abort(ex); + + return false; + } + + return true; + } + + private void DecrementQueuedFireAndForgetFrames() + { + int pending = Interlocked.Decrement(ref _queuedFireAndForgetFrames); + Debug.Assert(pending >= 0); + } + /// Abort all streams and cause further processing to fail. /// Exception causing Abort to be called. private void Abort(Exception abortException) @@ -2173,7 +2217,7 @@ private void VerifyKeepAlive() _keepAlivePingTimeoutTimestamp = now + _keepAlivePingTimeout; long pingPayload = Interlocked.Increment(ref _keepAlivePingPayload); - LogExceptions(SendPingAsync(pingPayload)); + QueuePing(pingPayload); return; } break; diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs index 470cacbe2de34c..359e78b1995e64 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs @@ -220,7 +220,7 @@ internal void OnDataOrHeadersReceived(Http2Connection connection, bool sendWindo // Send a PING _pingCounter--; if (NetEventSource.Log.IsEnabled()) connection.Trace($"[FlowControl] Sending RTT PING with payload {_pingCounter}"); - connection.LogExceptions(connection.SendPingAsync(_pingCounter, isAck: false)); + connection.QueuePing(_pingCounter, isAck: false); _pingSentTimestamp = now; _state = State.PingSent; } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 16463469d10986..bf62e5d0e070c3 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -3206,6 +3206,176 @@ await conn.WriteFrameAsync( await Task.WhenAll(connectionTasks).ConfigureAwait(false); } + [ConditionalFact(typeof(SocketsHttpHandlerTest_Http2), nameof(SupportsAlpn))] + public async Task Http2_SettingInFlightLimitExceeded() + { + // test that client abort connection when server send too much PING/SETTINGs while not processing ACK replies + + await Http2LoopbackServer.CreateClientAndServerAsync( + async uri => + { + using HttpClient client = CreateHttpClient(); + + Exception e = await Assert.ThrowsAsync(() => client.GetAsync(uri)); + Assert.True(e.Message.StartsWith(SR.net_http_http2_frame_limit_exceeded), "Bad Exception Message"); + + // test that we can open connection after failure + await client.GetAsync(uri); + }, + async server => + { + server.AllowMultipleConnections = true; + + await using Http2LoopbackConnection con = await server.AcceptConnectionAsync(); + while (true) + { + try + { + await con.WriteFrameAsync(new Frame(0, FrameType.Settings, FrameFlags.None, 0)); + } + catch (IOException) + { + break; + } + } + + // second connection will be more lucky + await server.AcceptConnectionSendResponseAndCloseAsync(HttpStatusCode.OK, "ok"); + }); + } + + [ConditionalFact(typeof(SocketsHttpHandlerTest_Http2), nameof(SupportsAlpn))] + public async Task Http2_PingInFlightLimitExceeded() + { + // almost exhaust client queue and test that following asynchronous PING issued by client trips + + BlockedWriteNetworkStream? clientNetStream = null; + + await Http2LoopbackServer.CreateClientAndServerAsync( + async uri => + { + using HttpClientHandler handler = CreateHttpClientHandler(allowAllCertificates: true); + SocketsHttpHandler socketsHandler = GetUnderlyingSocketsHttpHandler(handler); + + socketsHandler.ConnectCallback += async (ctx, ct) => + { + DnsEndPoint dnsEndPoint = ctx.DnsEndPoint; + IPAddress[] addresses = await Dns.GetHostAddressesAsync(dnsEndPoint.Host, dnsEndPoint.AddressFamily, ct); + + var s = new Socket(SocketType.Stream, ProtocolType.Tcp); + await s.ConnectAsync(addresses, dnsEndPoint.Port, ct); + + clientNetStream = new BlockedWriteNetworkStream(s, true); + return clientNetStream; + }; + socketsHandler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); + socketsHandler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.Always; + socketsHandler.KeepAlivePingTimeout = TimeSpan.MaxValue; + + using HttpClient client = CreateHttpClient(handler); + client.DefaultRequestVersion = HttpVersion.Version20; + client.DefaultVersionPolicy = HttpVersionPolicy.RequestVersionExact; + + Exception e = await Assert.ThrowsAsync(() => client.GetAsync(uri)); + Assert.True(e.Message.StartsWith(SR.net_http_http2_frame_limit_exceeded), "Bad Exception Message"); + + // test that we can connect new connection after fail + await client.GetAsync(uri); + }, + async server => + { + server.AllowMultipleConnections = true; + + await using Http2LoopbackConnection con = await server.AcceptConnectionAsync(); + + // wait for initial frames from client + await con.ReadSettingsAsync(); + await con.ReadRequestHeaderAsync(); + PingFrame firstPing = await con.ReadPingAsync(); + + long writeCntBeforeSettingsAck = clientNetStream.StartBlocking(); + + // ACK for following SETTINGS will attemp flushing queue on client side and block its processing + await con.WriteFrameAsync(new SettingsFrame()); + + // wait before client attemp to send ACK and channel get blocked. + // Without this wait it may combine SETTINGs ACK and following PING ACKs into + // single buffer which will cause dequeueing them from queue and we fail to + // preload queue to expect 1000 entries as required by test later. + while (clientNetStream.WriteCallCount == writeCntBeforeSettingsAck) + { + await Task.Delay(50); + } + + // fill the client queue with replies to PING, 1000 exactly fit the queue + for (long i = 0; i < 1000; i++) + { + await con.WriteFrameAsync(new PingFrame(i, FrameFlags.None, 0)); + } + + // now confirm ping we received soon after initiating connection to eneble new heart beat ping + // this should trip and close connection + await con.WriteFrameAsync(new PingFrame(firstPing.Data, FrameFlags.Ack, 0)); + + // test that server/client are operational after failure + await server.AcceptConnectionSendResponseAndCloseAsync(); + }, new Http2Options() + { + UseSsl = false, // SSL introduce buffering which supress effect of blocking + EnableTransparentPingResponse = false // need to observe initial ping + }); + } + + private class BlockedWriteNetworkStream : NetworkStream + { + private object _lock = new object(); + private bool _block; + private long _writes; + + public long WriteCallCount + { + get + { + lock (_lock) + { + return _writes; + } + } + } + + public BlockedWriteNetworkStream(Socket socket, bool ownsSocket) : base(socket, ownsSocket) { } + + public long StartBlocking() + { + lock (_lock) + { + _block = true; + return _writes; + } + } + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => + WriteAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask(); + + public override async ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + { + bool block; + lock (_lock) + { + _writes++; + block = _block; + } + + if (block) + { + await Task.Delay(TimeSpan.FromDays(1), cancellationToken); + throw new Exception("Long delay was canceled early"); + } + + await base.WriteAsync(buffer, cancellationToken); + } + } + private async Task VerifySendTasks(IReadOnlyList> sendTasks) { await TestHelper.WhenAllCompletedOrAnyFailed(sendTasks.ToArray()).ConfigureAwait(false);