Reintroduce progressive HTTP read timeout support - #15
Open
BGQ99 wants to merge 2 commits into
Open
Conversation
# Conflicts: # example/http_c++/http_client.cpp
Author
|
Additional validation evidence for commit Local stress testing
The attached archive contains:
Archive: progressive-read-timeout-test-evidence-fc865456.zip Archive SHA256:
GitHub Actions rerunsFive of six complete Linux workflow attempts passed. Attempt #5 failed only in the existing The other 10 Linux jobs in that attempt passed. This PR does not modify RDMA Validation ScreenshotsGitHub Actions attempt #6: all Linux jobs passed
|
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.



What problem does this PR solve?
Issue Number: Related to apache#3133, follow-up to apache#3409 and apache#3453
Problem Summary:
Progressive HTTP response reads currently have no independent idle timeout after
ReadProgressiveAttachmentBy()is called. A reader may therefore wait indefinitely when the peer stops sending body data while keeping the connection open.PR apache#3409 introduced progressive-read timeout support, but post-merge CI exposed a lifecycle and synchronization problem around the timer callback and reader completion path. In particular, the timer callback could race with reader completion/error delivery, which made
HttpTest.progressive_read_timeout_preserves_reader_errorunstable and could replace the reader's original error with the timeout status.This PR restores the feature with explicit synchronization and independent timer-state ownership.
What is changed and the side effects?
Changed:
Controller::set_progressive_read_timeout_ms()for progressively read HTTP/1.x response bodies.ReadProgressiveAttachmentBy()is called.reader_failed,timeout_triggered,end_delivered, timer ownership and user-callback state with the same mutex.OnEndOfMessage().Socket::SetFailed()instead of destroying the parsing context from the timer thread.SocketIdcarried byHttpContextto avoid operating on a recycled socket.EPROGREADTIMEOUTwhen an idle timeout occurs.ENOTSUP.Side effects:
Performance effects:
progressive_read_timeout_mskeep the original behavior.Breaking backward compatibility:
brpc::Controller, so applications should be rebuilt when upgrading instead of mixing binaries built against different brpc versions.Verification
Verified on commit
fc865456fcc21f2e8325e3e9b0c27f7d6fce7b21.Local focused stress tests:
GitHub Actions:
Detailed sanitized logs, reproduction scripts and SHA256 checksums will be attached in a PR comment.
Check List: