Skip to content

Make upstream_response_body_filter async, like its request-side counterpart #964

Description

@torinnd

What is the problem your feature solves, or the need it fulfills?

request_body_filter is async, and its documentation explains why: "The async nature of this function allows to throttle the upload speed and/or executing heavy computation logic such as WAF rules on offloaded threads without blocking the threads who process the requests themselves."

The response-side equivalent, upstream_response_body_filter, is sync. The workloads named in that doc comment apply at least as strongly to response bodies: content inspection (AV/DLP scanning against an external engine), and any per-chunk work that should be offloaded rather than run on the proxy thread. A filter that needs to await mid-body currently has to block in the sync filter, which stalls the worker for every connection on it, and buffering the body to handle at end-of-stream does not help because there is no async point there either.

Response bodies are also the only part of the response path with this restriction: the header filters (upstream_response_filter, response_filter) are already async, and the proxy itself awaits per chunk when it applies the pacing this filter returns (time::sleep on the returned Duration).

Describe the solution you'd like

async fn upstream_response_body_filter(
    &self,
    _session: &mut Session,
    _body: &mut Option<Bytes>,
    _end_of_stream: bool,
    _ctx: &mut Self::CTX,
) -> Result<Option<Duration>>
where
    Self::CTX: Send + Sync,
{
    Ok(None)
}

The signature otherwise stays the same: the Duration pacing mechanism is untouched, and the added bound matches the trait's other async methods. There is a single dispatch site, already inside an async fn, so the plumbing change is one .await. Since ProxyHttp is an #[async_trait] trait, existing implementors who override this method would add the async keyword, which is source-breaking but mechanical.

I have this working, with tests, on a branch: async-upstream-response-body-filter. Happy to send it as a PR if this direction is acceptable.

Benchmarks

This touches the hot path for everyone, including users who never override the filter, so I measured the cost in isolation and end to end.

First, a few caveats: the numbers come from a shared dev machine over loopback, with no CPU pinning, and I see 15-25% run-to-run noise on the end-to-end workloads.

I think these numbers are directionally interesting, if a bit imprecise. That said, the harness is reproducible if you want numbers from quieter hardware: proxy-filter-bench runs it against stock main and async-filter-bench against the change. The two copies differing only in the async keyword on two impls and one .await. cargo bench -p pingora-proxy on each. I'd also be happy to PR the harness separately if in-tree benchmarking would be useful.

My numbers, from interleaved run pairs:

  • Per-call dispatch in isolation: 13.8-14.1 ns/call sync, 22.9-23.0 ns/call async, so roughly +9 ns per body chunk. That is consistent with the boxed future #[async_trait] allocates per call. A trivial override costs the same as the default no-override case.
  • Calls per request, through a real proxy: an h2 upstream delivers one filter call per DATA frame (1280 calls for a 10 MiB body in 8 KiB frames); h1 coalesces reads (roughly 330-395 calls for the same body).
  • End to end (throughput on large chunked bodies, request rate on small ones, h1 and h2 upstreams) didn't exhibit a measurable difference. Scaling the per-call delta by the h2 call rate suggests something on the order of 10 us of added CPU per 10 MiB response, which is well below what this setup can resolve.

Describe alternatives you've considered

  • An additive parallel hook (an async variant whose default delegates to the sync one): avoids the source break, at the cost of two hooks for the same purpose and ambiguity about which one runs. If you prefer this shape it also works for my use case, and it confines the per-chunk cost to implementors who opt in.
  • Also making response_body_filter (the downstream one) async for full symmetry: I left this out on purpose. It has multiple call sites, including one in the synchronous cache-serving path, so it is a larger change with cache implications, and the upstream filter is the natural place for response inspection (before cache admission). It could follow later if you want it.
  • Doing the work outside ProxyHttp (wrapper service or custom app): re-implements the proxy loop and loses connection pooling, caching, and the rest of the filter pipeline.

Additional context

This is the first of two pieces needed for response content inspection (AV/DLP) in a ProxyHttp service. The second is a way to hold the response header back until the body verdict is reached, since a 200 that has already been sent cannot become a 403. I have a working implementation of that as well, building on this change; it has more design surface than this one, so if this direction is acceptable I would propose it separately.

Activity

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

Metadata

Metadata

Assignees

Labels

WIPWe are working on this feature internallyenhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions