Give a chance to send a response before receiving next request on H2 #9997
Conversation
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
Member
Author
|
[approve ci autest] |
Member
Author
|
|
duke8253
requested changes
Sep 6, 2023
| verify: {as: equal} | ||
|
|
||
| # Unrelated request just to keep the connection open | ||
| - client-request: |
Contributor
There was a problem hiding this comment.
Looks good. Going to wait for the tests to be changed to utilize the new PV feature that keeps connection open.
maskit
force-pushed
the
h2_respond_first
branch
from
dd5336f to
e73a996
Compare
Member
Author
|
Updated the tests. |
duke8253
approved these changes
Sep 7, 2023
cmcfarlen
pushed a commit
to cmcfarlen/trafficserver
that referenced
this pull request
Jun 3, 2024
* asf/master: (22 commits) fix: check whether a protocol is enabled during the length calculation in create_npn_advertisement (apache#10381) Coverity 1518612: Remove dead code (apache#10384) prefetch_cmcd: make autests more robust by removing need for gold file wildcard (apache#10382) Give a chance to send a response before receiving next request on H2 (apache#9997) CID 1516688: Fix uninitialized member of AcceptOptions (apache#10152) Fix slice head request memory issue (apache#10285) Fixes the TSMgmt metrics APIs for new API metrics (apache#10379) Minor parent.config a/an change (apache#10372) Allow DbgCtl tag to be set after instance construction. (apache#10375) Fix more build dep issues, for later PRs to work (apache#10376) money_trace cid 1518569: string not null terminated (apache#10373) Fix a couple of Coverity issues in health check plugin, around filenames (apache#10371) Fixes some build issues that happens with other changes (apache#10374) Eliminate unreachable code covered by switch default (apache#10370) Add tests for disk failure (apache#10192) Disable copying/moving for DbgCtl. (apache#10321) Cmake autest (apache#10327) cmake: add unit tests from mgmt/rpc (apache#10366) Adjust CMakeLists with git worktree (apache#10298) Fix example plugins build (apache#10326) ...
bneradt
added a commit
to bneradt/trafficserver
that referenced
this pull request
Jul 12, 2024
Revert "Give a chance to send a response before receiving next request on H2 (apache#9997)" This reverts commit 62abe06. I'm reverting this commit because it seems to introduce the crash seen in YTSATS-4273.
bneradt
pushed a commit
to bneradt/trafficserver
that referenced
this pull request
Jul 12, 2024
…pache#9997) * Give a chance to send a response before receiving next request on H2 * Keep client connections open to let ATS receive RST_STREAM * Use new ProxyVerifier that supports keep-connection-open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
ATS keeps reading H2 frames until either it reads all data in the receive buffer or it processes 128 H2 frames. This is not great if a client sends many requests at once. The first request will not be responded until ATS finishes/stops reading incoming frames, and that leads to a long TTFB. This also means ATS does not fully utilize a connection. ATS should be able to read following frames while response data is being transferred by Kernel/NIC.
This PR extends
Http2CommonSession::_should_do_something_else, so ATS can stop reading frames regardless of the number of frames it read. I addedHttp2CommonSession::interrupt_reading_frames(), and by calling it afterHttp2Stream::send_request, ATS can get a chance to send response data before reading following H2 frames.This benchmark was done on my laptop so rps and tps fluctuate and unreliable, but I can consistently see lower TTFB.
h2load -n 3000 -m 100 -c 10 "https://localhost:8443/256k"Before
After
And this is from production. A service that makes a lot of concurrent requests. Download time per host within a same pop, and the first row is the host with this change.
