fix: bind body_reader to the response it came from - #32
Open
shreemaan-abhishek wants to merge 1 commit into
Open
fix: bind body_reader to the response it came from#32shreemaan-abhishek wants to merge 1 commit into
shreemaan-abhishek wants to merge 1 commit into
Conversation
A reader held past its response read whatever the connection was on next. The op now carries a response generation that goes up on every request, every set_keepalive and every close. A reader captures the generation of the response that made it and hands it back on each read, and a read whose generation is no longer the current one is refused with "stale body reader". The check lives in C, next to the state it guards, so an FFI caller driving the module directly cannot read past a response either. ffi_read_body and ffi_get_trailers take the generation as a parameter and ffi_get_generation hands out the current one. The hazard was reachable: a preread response leaves its reader unused, so after a second request that reader used to return the second response's body and left the second response empty. Closes #26. Part of #18.
📝 WalkthroughWalkthroughThe client now assigns generations to responses. Body readers and trailer access validate their captured generation in Lua and C. Stale readers return ChangesStale reader validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ngx_http_ffi_client_request.c`:
- Around line 2406-2422: Update ngx_http_ffi_client_ffi_close to handle any
parked op->pending read before clearing cleanup or closing the connection:
reject it or resume its coroutine with the "connection closed" error, then clear
the pending state. Ensure no pending body reader remains suspended indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 012d1063-f1b6-4a89-86c3-3445c0b89724
📒 Files selected for processing (5)
README.mdlib/resty/ngx_http_ffi_client.luasrc/ngx_http_ffi_client.hsrc/ngx_http_ffi_client_request.ct/016-body-reader-generation.t
Comment on lines
2406
to
+2422
| ngx_int_t | ||
| ngx_http_ffi_client_ffi_read_body(ngx_http_request_t *r, | ||
| ngx_http_ffi_client_op_t *op, ngx_http_ffi_client_ffi_chunk_t *chunk, | ||
| u_char *err, size_t *errlen) | ||
| ngx_http_ffi_client_op_t *op, ngx_uint_t generation, | ||
| ngx_http_ffi_client_ffi_chunk_t *chunk, u_char *err, size_t *errlen) | ||
| { | ||
| ngx_int_t rc; | ||
|
|
||
| if (op == NULL || chunk == NULL) { | ||
| return ngx_http_ffi_client_set_err(err, errlen, "bad read"); | ||
| } | ||
|
|
||
| /* the response this reader was made for is gone, so reading now would hand | ||
| * back bytes of whatever response the op moved on to */ | ||
| if (generation != op->generation) { | ||
| return ngx_http_ffi_client_set_err(err, errlen, "stale body reader"); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how a pending READ_BODY operation is resumed/completed,
# and whether close()/set_keepalive() guard against a pending operation.
set -euo pipefail
# Find the resume path for a parked read (posted read event -> handler).
rg -n "PENDING_READ_BODY" src/ngx_http_ffi_client_request.c -C 15
# Find the event handler(s) that call ngx_http_ffi_client_next_chunk or resume
# the coroutine after NGX_AGAIN, to see if generation is re-validated there.
rg -n "ngx_http_ffi_client_next_chunk|resumed = 1|ngx_http_lua_run_posted_threads|ngx_http_ffi_client_handle_read" src/ngx_http_ffi_client_request.c -C 10
# Confirm whether ffi_close guards on op->pending anywhere else in the codebase.
rg -n "ngx_http_ffi_client_ffi_close" src/*.c src/*.h -C 5Repository: api7/ngx_http_ffi_client
Length of output: 12908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- operation handlers and pending-read completion ---'
sed -n '1320,1545p' src/ngx_http_ffi_client_request.c
sed -n '1800,1975p' src/ngx_http_ffi_client_request.c
printf '%s\n' '--- FFI read/keepalive/close paths ---'
sed -n '2390,2625p' src/ngx_http_ffi_client_request.c
printf '%s\n' '--- all pending-state transitions and generation updates ---'
rg -n "op->pending|op->generation|ngx_http_ffi_client_finalize|ngx_http_ffi_client_ffi_close" src/ngx_http_ffi_client_request.cRepository: api7/ngx_http_ffi_client
Length of output: 22481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- event handlers that dispatch pending operations ---'
sed -n '1000,1140p' src/ngx_http_ffi_client_request.c
printf '%s\n' '--- operation setup, generation capture, and response completion ---'
sed -n '2000,2120p' src/ngx_http_ffi_client_request.c
sed -n '2240,2400p' src/ngx_http_ffi_client_request.c
printf '%s\n' '--- active close implementation and event cancellation ---'
rg -n "ngx_http_ffi_client_keepalive_close_active|keepalive_close_active|ngx_close_connection|ngx_del_event|read->handler|write->handler" src -C 8Repository: api7/ngx_http_ffi_client
Length of output: 20271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- full active-close implementation ---'
sed -n '250,285p' src/ngx_http_ffi_client_keepalive.c
sed -n '520,552p' src/ngx_http_ffi_client_keepalive.c
printf '%s\n' '--- FFI close callers and Lua/API exposure ---'
rg -n "ffi_close|client.*close|close\\(" . -g '*.[ch]' -g '*.lua' -g '*.md' -g '*.yml' -g '*.yaml' -C 6
printf '%s\n' '--- pending operation and coroutine ownership fields ---'
rg -n "typedef struct ngx_http_ffi_client_op_s|coctx|stateful|resumed|pending" src/ngx_http_ffi_client.h src/ngx_http_ffi_client_request.c -C 4Repository: api7/ngx_http_ffi_client
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Lua read and close control flow ---'
sed -n '780,960p' lib/resty/ngx_http_ffi_client.lua
printf '%s\n' '--- client object sharing and coroutine-related documentation ---'
rg -n "body_reader|read_body|set_keepalive|close|coroutine|coroutine.wrap|ngx.thread|shared|same client|client object" README.md lib t test tests 2>/dev/null -C 5 || true
printf '%s\n' '--- event deletion and close usage in this repository ---'
rg -n "ngx_delete_posted_event|ngx_post_event|ngx_close_connection|keepalive_close_active" src -C 5Repository: api7/ngx_http_ffi_client
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- body-reader resume behavior ---'
sed -n '700,742p' lib/resty/ngx_http_ffi_client.lua
printf '%s\n' '--- close and pending-read relevant source slices ---'
sed -n '1085,1130p' src/ngx_http_ffi_client_request.c
sed -n '2410,2485p' src/ngx_http_ffi_client_request.c
sed -n '2587,2610p' src/ngx_http_ffi_client_request.c
printf '%s\n' '--- deterministic source-level invariant check ---'
python3 - <<'PY'
from pathlib import Path
request = Path("src/ngx_http_ffi_client_request.c").read_text()
keepalive = Path("src/ngx_http_ffi_client_keepalive.c").read_text()
read_start = request.index("ngx_http_ffi_client_ffi_read_body(")
read_end = request.index("/*\n * Trailers", read_start)
read = request[read_start:read_end]
handler_start = request.index("static void\nngx_http_ffi_client_read_handler")
handler_end = request.index("/*\n * Report the pending socket error", handler_start)
handler = request[handler_start:handler_end]
close_start = request.index("void\nngx_http_ffi_client_ffi_close")
close_end = request.index("\n}", close_start) + 2
close = request[close_start:close_end]
active_start = keepalive.index("void\nngx_http_ffi_client_keepalive_close_active")
active_end = keepalive.index("\n}", active_start) + 2
active = keepalive[active_start:active_end]
checks = {
"read parks as PENDING_READ_BODY": "op->pending = NGX_HTTP_FFI_CLIENT_PENDING_READ_BODY;" in read,
"close clears pending": "op->pending = NGX_HTTP_FFI_CLIENT_PENDING_NONE;" in close,
"close clears peer connection through active close": "op->peer.connection = NULL;" in active,
"read handler returns when stateful op has no pending operation":
"if (op->stateful && op->pending == NGX_HTTP_FFI_CLIENT_PENDING_NONE)" in handler
and "return;" in handler,
"read handler has no generation comparison":
"generation" not in handler,
}
for name, ok in checks.items():
print(f"{name}: {'yes' if ok else 'no'}")
PYRepository: api7/ngx_http_ffi_client
Length of output: 5251
Resume or reject pending reads in ngx_http_ffi_client_ffi_close. close() clears op->pending and the cleanup while a body reader is parked. The read handler then returns without resuming the coroutine, which can leave it suspended indefinitely. Make close() reject pending operations or resume the pending read with "connection closed" before closing the connection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ngx_http_ffi_client_request.c` around lines 2406 - 2422, Update
ngx_http_ffi_client_ffi_close to handle any parked op->pending read before
clearing cleanup or closing the connection: reject it or resume its coroutine
with the "connection closed" error, then clear the pending state. Ensure no
pending body reader remains suspended indefinitely.
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness hazard in the stateful client where res.body_reader could outlive its response and accidentally consume the body (and affect consumption) of a later response on the same connection. It introduces a monotonically advancing “response generation” tracked in C and captured by each Lua body reader so stale reads are refused with nil, "stale body reader".
Changes:
- Add an
op->generationcounter in the C op state; bump it onrequest,set_keepalive, andclose, and require callers to provide the expected generation when reading bodies/trailers. - Bind Lua
body_readerclosures to the response generation (and pass it through to C on every read), preventing stale readers from consuming later responses. - Add Test::Nginx coverage for stale-reader scenarios plus documentation describing reader expiration semantics.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
t/016-body-reader-generation.t |
Adds regression tests covering stale readers across later requests, keepalive, close, and reconnect, plus a happy-path streaming case. |
src/ngx_http_ffi_client.h |
Adds generation to the op struct and updates FFI function signatures to carry generation for body/trailer reads. |
src/ngx_http_ffi_client_request.c |
Implements generation bumping and enforces generation matching in ffi_read_body / ffi_get_trailers, plus exposes ffi_get_generation. |
lib/resty/ngx_http_ffi_client.lua |
Captures the response generation at request time and supplies it on each chunk read / trailer fetch. |
README.md |
Documents that body readers expire once the connection advances and will return nil, "stale body reader". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2384
to
2388
| op->received = 0; | ||
| op->request_sent = 1; | ||
| /* a new response starts here, so a reader of the previous one is stale */ | ||
| op->generation++; | ||
| op->resumed = 0; |
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.
Closes #26. Part of #18.
Problem
res.body_readerwas not tied to the response it came from, so a reader held past its response read whatever the connection moved on to.The hazard is reachable today. A
preread_bodyresponse leaves its reader unused, so after a second request that reader returned the second response's body and left the second response empty:The same held for a reader kept across
set_keepalive()orclose(): it kept handing out the old response's bytes.Fix
The op carries a response generation that only goes up: every
request, everyset_keepaliveand everyclosemoves it. A reader captures the generation of the response that produced it and hands it back on every read; a read whose generation is no longer current is refused withstale body reader.The check lives in C, next to the state it guards, so an FFI caller driving the module directly cannot read past a response either.
ffi_read_bodyandffi_get_trailerstake the generation as a parameter, andffi_get_generationhands out the current one.Tests
t/016-body-reader-generation.t, six cases: a stale reader after a later request,read_bodyon a stale response, a reader kept acrossset_keepalive, acrossclose, across a reconnect, and the happy path with one reader per response.Verified locally against OpenResty 1.31.1.1 on both parser backends:
The new file fails 5 of its 18 subtests when built without the fix, with the stale reader returning the later response's body, and passes with it.
Summary by CodeRabbit
Bug Fixes
"stale body reader"error after request transitions, keepalive, reconnects, or connection closure.Documentation