api7 / api7/ngx_http_ffi_client

Hand-written streaming parser retains the whole response in the request pool

Open
#27 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

area:perf bug P2
Dominant language
C
Stars
2
Forks
0
PR merge metrics
No merged PRs in 30d

Description

P2 follow-up from the review of #22 (https://github.com/api7/ngx_http_ffi_client/pull/22#issuecomment-5088042507).

In streaming mode the hand-written parser backend (NGX_HTTP_FFI_CLIENT_USE_LLHTTP=0) still accumulates the entire response, which defeats the point of incremental reads.

Detail

ngx_http_ffi_client_response_append() (src/ngx_http_ffi_client_response.c:198-230) appends every received byte into parser->raw, growing it by doubling into the request pool — and an nginx pool never frees the old buffers, so a grown raw costs the sum of all its generations. Consumed bytes are never reclaimed: ngx_http_ffi_client_response_update_body() (:480-510) only advances parser->taken as a cursor into the still-complete buffer.

Result: streaming a 1 GB body or a long-lived SSE stream on the fallback backend holds the whole thing in the request pool until the request ends, even though the caller copied and dropped each chunk. The llhttp backend hands spans out of the read buffer instead (src/ngx_http_ffi_client_response_llhttp.c:384-405), so this is fallback-specific.

Proposal

Either:

  • make the fallback streaming path memory-bounded — compact/reset raw once a span has been handed out and taken catches up, so peak usage is bounded by the read buffer plus one pending chunk; or
  • accept that the hand-written parser is a migration-only fallback (README "Project status" §2) and refuse stream mode on it with a clear error rather than silently buffering everything.

Whichever is chosen should be stated in the README so the two backends' streaming guarantees are not silently different.

Acceptance

  • A streaming test on the fallback backend over a body far larger than the read buffer shows bounded memory (or a clean, documented refusal).
  • Both backends' streaming semantics are documented.

Related: README "Project status" §2, #12. Part of #18.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with ngx_http_ffi_client_response_append() and ngx_http_ffi_client_response_update_body() in src/ngx_http_ffi_client_response.c, then compare the streaming spans in src/ngx_http_ffi_client_response_llhttp.c. Check the README Project status §2 and related issue #12 before choosing between bounded fallback buffering and refusing stream mode. Done means a fallback streaming test demonstrates bounded memory or a clear refusal, with both backend semantics documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, nginx
Domain
api, backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.