Skip to content

Reintroduce progressive HTTP read timeout support - #15

Open
BGQ99 wants to merge 2 commits into
LinQuickDev:progressive-timeout-v2from
BGQ99:review/pr3409-final
Open

Reintroduce progressive HTTP read timeout support#15
BGQ99 wants to merge 2 commits into
LinQuickDev:progressive-timeout-v2from
BGQ99:review/pr3409-final

Conversation

@BGQ99

@BGQ99 BGQ99 commented Aug 20, 2026

Copy link
Copy Markdown

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_error unstable 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:

  • Add Controller::set_progressive_read_timeout_ms() for progressively read HTTP/1.x response bodies.
  • Treat the configured value as the maximum idle interval between successfully consumed body parts.
  • Start the watchdog when ReadProgressiveAttachmentBy() is called.
  • Refresh the idle deadline after every successful reader callback.
  • Do not count time spent inside the user reader callback as idle time.
  • Preserve an error returned by the user reader instead of replacing it with a later timeout.
  • Keep timer state independently alive with shared ownership, so the timer callback does not access a destroyed reader wrapper.
  • Protect reader_failed, timeout_triggered, end_delivered, timer ownership and user-callback state with the same mutex.
  • Cancel or detach the watchdog before delivering OnEndOfMessage().
  • Close the HTTP/1.x socket through Socket::SetFailed() instead of destroying the parsing context from the timer thread.
  • Use the versioned SocketId carried by HttpContext to avoid operating on a recycled socket.
  • Return EPROGREADTIMEOUT when an idle timeout occurs.
  • Reject progressive-read timeout for HTTP/2 with ENOTSUP.
  • Update the HTTP client/server examples.
  • Add focused unit tests covering:
    • active progressive reads;
    • idle timeout after receiving data;
    • timeout before the first body part;
    • slow user callbacks;
    • preservation of reader errors;
    • rejection for HTTP/2.

Side effects:

  • Performance effects:

    • The feature is disabled by default.
    • When enabled, one watchdog timer and a small shared state object are created for the progressive reader.
    • Reader/timer state transitions use a mutex.
    • Existing calls that do not configure progressive_read_timeout_ms keep the original behavior.
  • Breaking backward compatibility:

    • No source-level behavior change when the option is unset.
    • The new fields change the layout of 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:

  • ASan: 200/200 rounds passed.
  • 6 tests per round, 1,200 total test invocations.
  • 0 test failures.
  • 0 AddressSanitizer errors.
  • Bazel 8.3.1: 20/20 runs passed.
  • 6 tests per run, 120 total test invocations.

GitHub Actions:

  • License Check: passed.
  • Build on macOS: passed.
  • Build and Test on Linux: passed.
  • The latest complete Linux attempt passed all 11 jobs, including ASan and Bazel jobs.

Detailed sanitized logs, reproduction scripts and SHA256 checksums will be attached in a PR comment.


Check List:

  • The changes are compilable.
  • Focused unit tests were added.
  • ASan stress testing passed.
  • Bazel repeated testing passed.
  • GitHub Actions passed.
  • The changes follow the Contributor Covenant Code of Conduct.

@BGQ99

BGQ99 commented Aug 20, 2026

Copy link
Copy Markdown
Author

Additional validation evidence for commit fc865456fcc21f2e8325e3e9b0c27f7d6fce7b21.

Local stress testing

  • ASan: 200/200 rounds passed.
  • 6 focused progressive-read timeout tests per round.
  • 1,200 total test invocations.
  • 0 failures.
  • 0 sanitizer errors.
  • Bazel 8.3.1: 20/20 runs passed.
  • 120 total focused test invocations.

The attached archive contains:

  • sanitized ASan log;
  • sanitized Bazel log;
  • ASan reproduction script;
  • Bazel reproduction script;
  • SHA256 checksums for every included file.

Archive:

progressive-read-timeout-test-evidence-fc865456.zip

Archive SHA256:

3641592B51D1BCE19F5BA1796FB2F91C27E5980D94CC9E040847D16AF2CFBAA4

GitHub Actions reruns

Five of six complete Linux workflow attempts passed.

Attempt #5 failed only in the existing
clang-unittest-bazel-with-babylon-and-new-pb job, in
RdmaTest.v3_server_rejects_oversized_pb_size.

The other 10 Linux jobs in that attempt passed. This PR does not modify RDMA
code or that test. The subsequent complete attempt #6 passed all 11 Linux jobs,
including the previously failing job.

Validation Screenshots

ASan stress test: 200/200 rounds passed asan
Bazel 8.3.1 stress test: 20/20 runs passed bazel
GitHub Actions attempt #6: all Linux jobs passed CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants