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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
