Add a limit on number of pending HTTP/2 PING ACKs#130997
Open
mrek-msft wants to merge 1 commit into
Open
Conversation
|
Azure Pipelines: Successfully started running 4 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a safety limit to prevent an HTTP/2 connection from accumulating an unbounded number of queued “fire-and-forget” control frames (PINGs and SETTINGS ACKs), aborting the connection once the queue exceeds an internal threshold, and adds functional tests to validate the behavior.
Changes:
- Add a queued-frame counter and queueing helpers in
Http2Connectionthat abort the connection when the limit is exceeded. - Route RTT-estimator PINGs and keep-alive PINGs through the new queueing path.
- Add a new localized error string and functional tests that exercise SETTINGS/PING queue exhaustion scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs | Adds functional tests for SETTINGS/PING queue limit behavior and introduces a test stream used to block client writes. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs | Switches RTT PING send path to use the new queueing helper. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs | Implements the queue limit, queueing helpers, and abort behavior when the limit is exceeded. |
| src/libraries/System.Net.Http/src/Resources/Strings.resx | Adds a new error string for the queue-limit abort. |
Comment on lines
+3209
to
+3211
| [ConditionalFact(typeof(SocketsHttpHandlerTest_Http2), nameof(SupportsAlpn))] | ||
| public async Task Http2_SettingInFlightLimitExceeded() | ||
| { |
Comment on lines
+3212
to
+3213
| // test that client abort connection when server send too much PING/SETTINGs while not processing ACK replies | ||
|
|
Comment on lines
+3296
to
+3308
| 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); | ||
| } |
Comment on lines
+3324
to
+3326
| UseSsl = false, // SSL introduce buffering which supress effect of blocking | ||
| EnableTransparentPingResponse = false // need to observe initial ping | ||
| }); |
Comment on lines
+3369
to
+3376
| if (block) | ||
| { | ||
| await Task.Delay(TimeSpan.FromDays(1), cancellationToken); | ||
| throw new Exception("Long delay was canceled early"); | ||
| } | ||
|
|
||
| await base.WriteAsync(buffer, cancellationToken); | ||
| } |
Comment on lines
+426
to
+428
| <data name="net_http_http2_frame_limit_exceeded" xml:space="preserve"> | ||
| <value>The HTTP/2 connection was aborted because the size of the frame queue exceeded an internal limit.</value> | ||
| </data> |
Comment on lines
+78
to
+80
| // Cap number of untransmitted PING and SETTING ACKs and PING requests | ||
| private const int MaxQueuedFireAndForgetFrames = 1000; | ||
| private int _queuedFireAndForgetFrames; |
Comment on lines
+1840
to
+1854
| 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; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.