You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "shreemaan-abhishek (via GitHub)" <gi...@apache.org> on 2026/09/01 14:55:45 UTC
[PR] fix(batch-requests): bound aggregated response bodies [apisix]
shreemaan-abhishek opened a new pull request, #13906:
URL: https://github.com/apache/apisix/pull/13906
### Description
The `batch-requests` endpoint retains pipeline response bodies before serializing the aggregate. Large responses can make the total buffered data grow well beyond the input size.
This change:
- adds `max_response_body_size` (1 MiB by default) and `max_response_body_size_total` (10 MiB by default) to plugin metadata
- checks `Content-Length` before reading when it is available and enforces both limits while reading chunks
- closes the pipeline connection and returns 502 with the pipeline item index when either limit is reached
- adds coverage for per-item, aggregate, exact-boundary, and metadata validation behavior
#### Behavior change and metadata configuration
This intentionally changes behavior for pipelines whose response bodies exceed the new defaults: they now return 502 instead of returning the full aggregate. Existing plugin metadata needs no data migration because the new fields use defaults when absent. Deployments that depend on larger response bodies should set `max_response_body_size` and `max_response_body_size_total` in `batch-requests` plugin metadata before rollout.
#### Which issue(s) this PR fixes:
N/A
### Tests
- `luacheck apisix/plugins/batch-requests.lua`
- `APISIX_HOME=/tmp/brl-apisix TEST_NGINX_SERVROOT=/tmp/brl-apisix/t/servroot prove -I. t/plugin/batch-requests.t`
- `git diff --check`
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(batch-requests): bound aggregated response bodies [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek merged PR #13906:
URL: https://github.com/apache/apisix/pull/13906
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(batch-requests): bound aggregated response bodies [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #13906:
URL: https://github.com/apache/apisix/pull/13906#discussion_r3920318265
##########
apisix/plugins/batch-requests.lua:
##########
@@ -224,18 +240,80 @@ local function set_common_query(data)
end
+local function close_http_client(httpc)
+ local ok, err = httpc:close()
+ if not ok then
+ core.log.warn("failed to close batch request connection: ", err)
+ end
+end
+
+
+local function read_response_body(httpc, resp, max_response_body_size,
+ response_body_size_total,
+ max_response_body_size_total)
+ local content_length = tonumber(resp.headers["Content-Length"])
+ if content_length then
+ if content_length > max_response_body_size then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size"
+ end
+
+ if response_body_size_total + content_length > max_response_body_size_total then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size_total"
+ end
+ end
+
+ local chunks = {}
+ local response_body_size = 0
+ while true do
+ local chunk, err = resp.body_reader(response_body_chunk_size)
+ if err then
Review Comment:
[P2] Preserve valid EOF-delimited response bodies
Please fix this before merging. For a response with neither Content-Length nor chunked transfer encoding, api7-lua-resty-http 0.2.3 uses connection closure to delimit the body. With body_reader(8192), its final return is partial, "closed". This branch treats that valid EOF as an error before consuming the partial data and discards the body accumulated so far.
A focused reproduction using the pinned library's actual reader and this patch's read_response_body returns "hello" with the old read_body() path, but nil, "closed" with the new path, even though the five-byte body is below both limits. The aggregate still returns HTTP 200, with read_body_err: "closed" and no body for that item.
This affects close-delimited internal responses, for example a dynamically generated response without Content-Length when chunked transfer encoding is disabled. Normal Content-Length and chunked responses do not trigger this issue.
Please distinguish a valid EOF from premature truncation, account for the final partial data against both limits, and add a gateway regression for close-delimited responses. Keep truncation errors for fixed-length and chunked responses intact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(batch-requests): bound aggregated response bodies [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #13906:
URL: https://github.com/apache/apisix/pull/13906#discussion_r3921185088
##########
apisix/plugins/batch-requests.lua:
##########
@@ -224,18 +240,80 @@ local function set_common_query(data)
end
+local function close_http_client(httpc)
+ local ok, err = httpc:close()
+ if not ok then
+ core.log.warn("failed to close batch request connection: ", err)
+ end
+end
+
+
+local function read_response_body(httpc, resp, max_response_body_size,
+ response_body_size_total,
+ max_response_body_size_total)
+ local content_length = tonumber(resp.headers["Content-Length"])
+ if content_length then
+ if content_length > max_response_body_size then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size"
+ end
+
+ if response_body_size_total + content_length > max_response_body_size_total then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size_total"
+ end
+ end
+
+ local chunks = {}
+ local response_body_size = 0
+ while true do
+ local chunk, err = resp.body_reader(response_body_chunk_size)
+ if err then
Review Comment:
Fixed in 485d61eb9. `closed` is now accepted only for close-delimited responses, and the final partial chunk is checked against both limits before being retained. Added focused close-delimited boundary tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(batch-requests): bound aggregated response bodies [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #13906:
URL: https://github.com/apache/apisix/pull/13906#discussion_r3921526315
##########
apisix/plugins/batch-requests.lua:
##########
@@ -224,18 +240,87 @@ local function set_common_query(data)
end
+local function close_http_client(httpc)
+ local ok, err = httpc:close()
+ if not ok then
+ core.log.warn("failed to close batch request connection: ", err)
+ end
+end
+
+
+local function read_response_body(httpc, resp, max_response_body_size,
+ response_body_size_total,
+ max_response_body_size_total)
+ local content_length = tonumber(resp.headers["Content-Length"])
+ local close_delimited = not content_length and
+ not http.transfer_encoding_is_chunked(resp.headers)
+ if content_length then
+ if content_length > max_response_body_size then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size"
+ end
+
+ if response_body_size_total + content_length > max_response_body_size_total then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size_total"
+ end
+ end
+
+ local chunks = {}
+ local response_body_size = 0
+ while true do
+ local chunk, err = resp.body_reader(response_body_chunk_size)
+ local valid_eof = close_delimited and err == "closed"
Review Comment:
`valid_eof` only recognises `err == "closed"`, but that is the sole error the close-delimited branch of lua-resty-http's `_body_reader` surfaces through the reader. Every other socket failure — a timeout under the caller-supplied `data.timeout`, most notably — takes this path:
```lua
local str, err, partial = sock:receive(max_chunk_size)
if not str and err == "closed" then
co_yield(partial, err)
end
max_chunk_size = tonumber(co_yield(str) or default_chunk_size) -- yields nil, no err
```
The loop here then sees `chunk == nil, err == nil`, breaks below, and returns the partial body as a success. The caller never sets `read_body_err`, and the batch returns 200.
A/B against `76ff252b1`, with a `chunked_transfer_encoding off` upstream that flushes and then stalls past `"timeout": 300`:
- master: `[error] ... read pipeline response body failed: timeout`, and `read_body_err` present in the aggregate
- this branch: `[{"headers":{...},"body":"","status":200,"reason":"OK"}]` — no error signal at all
A caller receives a truncated body that looks like a complete successful response, which is harder to diagnose than the memory growth this PR sets out to bound.
The `valid_eof` test needs to distinguish "reader finished" from "reader gave up" — either by tracking whether anything was yielded in this iteration, or by not entering the streaming path at all when `Content-Length` is known. The latter is worth considering on its own: once the length is checked up front there is nothing left to enforce while reading, yet the body is still pulled 8192 bytes at a time, so a 1 MiB response becomes ~128 `sock:receive` calls instead of one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(batch-requests): bound aggregated response bodies [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #13906:
URL: https://github.com/apache/apisix/pull/13906#discussion_r3946697781
##########
apisix/plugins/batch-requests.lua:
##########
@@ -224,18 +240,87 @@ local function set_common_query(data)
end
+local function close_http_client(httpc)
+ local ok, err = httpc:close()
+ if not ok then
+ core.log.warn("failed to close batch request connection: ", err)
+ end
+end
+
+
+local function read_response_body(httpc, resp, max_response_body_size,
+ response_body_size_total,
+ max_response_body_size_total)
+ local content_length = tonumber(resp.headers["Content-Length"])
+ local close_delimited = not content_length and
+ not http.transfer_encoding_is_chunked(resp.headers)
+ if content_length then
+ if content_length > max_response_body_size then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size"
+ end
+
+ if response_body_size_total + content_length > max_response_body_size_total then
+ close_http_client(httpc)
+ return nil, nil, "max_response_body_size_total"
+ end
+ end
+
+ local chunks = {}
+ local response_body_size = 0
+ while true do
+ local chunk, err = resp.body_reader(response_body_chunk_size)
+ local valid_eof = close_delimited and err == "closed"
Review Comment:
Fixed in 48554ceca and 254353741. The released client version 0.2.4 now preserves `partial, err` for close-delimited read failures, and the new regression confirms a timeout is returned as `read_body_err` instead of accepting a truncated body.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org