diff --git a/README.md b/README.md index 6cd5b6e..3e77217 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,12 @@ the whole body and resumes the coroutine once, with `res.body` ready. Use it for bounded bodies; a stream read with `body_reader` is the right call for SSE and very large responses. +A `body_reader` belongs to the response it came from. Once the connection has +moved on, through another `request`, a `set_keepalive` or a `close`, the reader +returns `nil, "stale body reader"` rather than bytes of whatever response came +after it. The check lives in C, so it holds for an FFI caller that drives the +module directly as well. + ### Headers `res.headers` and `res.trailers` follow the `lua-resty-http` contract. diff --git a/lib/resty/ngx_http_ffi_client.lua b/lib/resty/ngx_http_ffi_client.lua index 6c3963f..d9046e7 100644 --- a/lib/resty/ngx_http_ffi_client.lua +++ b/lib/resty/ngx_http_ffi_client.lua @@ -121,13 +121,16 @@ if not pcall(ffi.typeof, "ngx_http_ffi_client_ffi_req_t") then ngx_http_ffi_client_ffi_resp_t *resp, unsigned char *err, size_t *errlen); int 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, - unsigned char *err, size_t *errlen); + ngx_http_ffi_client_op_t *op, uintptr_t generation, + ngx_http_ffi_client_ffi_chunk_t *chunk, unsigned char *err, + size_t *errlen); int ngx_http_ffi_client_ffi_set_keepalive(ngx_http_request_t *r, ngx_http_ffi_client_op_t *op, uintptr_t idle_timeout, uintptr_t pool_size, unsigned char *err, size_t *errlen); void ngx_http_ffi_client_ffi_get_trailers(ngx_http_ffi_client_op_t *op, - ngx_http_ffi_client_ffi_resp_t *resp); + uintptr_t generation, ngx_http_ffi_client_ffi_resp_t *resp); + uintptr_t ngx_http_ffi_client_ffi_get_generation( + ngx_http_ffi_client_op_t *op); uintptr_t ngx_http_ffi_client_ffi_get_reused_times( ngx_http_ffi_client_op_t *op); void ngx_http_ffi_client_ffi_close(ngx_http_ffi_client_op_t *op); @@ -471,7 +474,7 @@ local res_mt local c_loaded local c_new, c_set_timeouts, c_connect, c_send, c_read, c_keepalive -local c_reused, c_close, c_trailers +local c_reused, c_close, c_trailers, c_generation local function load_stateful() @@ -489,6 +492,7 @@ local function load_stateful() c_reused = C.ngx_http_ffi_client_ffi_get_reused_times c_close = C.ngx_http_ffi_client_ffi_close c_trailers = C.ngx_http_ffi_client_ffi_get_trailers + c_generation = C.ngx_http_ffi_client_ffi_get_generation end) if not ok then @@ -667,9 +671,10 @@ function client.connect(self, opts, port_arg) end --- One body slice. The C side hands back a pointer into its read buffer that is --- only valid until the next call, so the copy into a Lua string happens here. -local function read_chunk(self) +-- One body slice of the response `generation` names. The C side hands back a +-- pointer into its read buffer that is only valid until the next call, so the +-- copy into a Lua string happens here. +local function read_chunk(self, generation) local r = get_request() if not r then error("no request found", 2) @@ -678,7 +683,7 @@ local function read_chunk(self) local chunk = self._chunk local err, errlen = new_err_buf() - local rc = c_read(r, self._op, chunk, err, errlen) + local rc = c_read(r, self._op, generation, chunk, err, errlen) if rc == FFI_ERROR then return nil, ffi_str(err, errlen[0]) @@ -702,7 +707,11 @@ local function read_chunk(self) end -local function make_body_reader(res, resp) +-- A reader is bound to the response it came from by its generation. The C side +-- refuses a read whose generation is no longer the one the connection is on, +-- so a reader kept past its response ("stale body reader") can never be handed +-- a later response's bytes. +local function make_body_reader(res, resp, generation) local self = res._httpc local done = false @@ -711,7 +720,7 @@ local function make_body_reader(res, resp) return nil end - local chunk, err, eof = read_chunk(self) + local chunk, err, eof = read_chunk(self, generation) if err then done = true @@ -722,7 +731,7 @@ local function make_body_reader(res, resp) done = true -- trailers arrive behind the last chunk, so they are only readable -- now that the body is fully consumed - c_trailers(self._op, resp) + c_trailers(self._op, generation, resp) res.trailers = header_table(resp.trailers, resp.trailers_len) end @@ -866,7 +875,8 @@ function client.request(self, params) res.trailers = header_table(resp.trailers, resp.trailers_len) end - res.body_reader = make_body_reader(res, resp) + res.body_reader = make_body_reader(res, resp, + tonumber(c_generation(self._op))) return res end diff --git a/src/ngx_http_ffi_client.h b/src/ngx_http_ffi_client.h index dc4e256..e1095b0 100644 --- a/src/ngx_http_ffi_client.h +++ b/src/ngx_http_ffi_client.h @@ -271,6 +271,13 @@ struct ngx_http_ffi_client_op_s { size_t errlen; /* stateful-object state; the one-shot path leaves all of it zero */ ngx_http_ffi_client_pending_e pending; + /* + * Which response the op is currently on. It only ever goes up: every + * request, every set_keepalive and every close moves it, so a body reader + * that captured an older value can be told apart from one reading the + * response it was made for. 0 means no response yet. + */ + ngx_uint_t generation; ngx_http_ffi_client_ffi_chunk_t *ffi_chunk; ngx_str_t host; u_char *rbuf; @@ -371,11 +378,18 @@ ngx_int_t ngx_http_ffi_client_ffi_connect(ngx_http_request_t *r, ngx_int_t ngx_http_ffi_client_ffi_send_request(ngx_http_request_t *r, ngx_http_ffi_client_op_t *op, ngx_http_ffi_client_ffi_reqp_t *rp, ngx_http_ffi_client_ffi_resp_t *resp, u_char *err, size_t *errlen); +/* + * generation names the response the caller means to read. It comes from + * ffi_get_generation right after the request that produced it, and a call + * carrying a stale one is refused instead of reading a later response's bytes. + */ 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); void ngx_http_ffi_client_ffi_get_trailers(ngx_http_ffi_client_op_t *op, - ngx_http_ffi_client_ffi_resp_t *resp); + ngx_uint_t generation, ngx_http_ffi_client_ffi_resp_t *resp); +ngx_uint_t ngx_http_ffi_client_ffi_get_generation( + ngx_http_ffi_client_op_t *op); ngx_int_t ngx_http_ffi_client_ffi_set_keepalive(ngx_http_request_t *r, ngx_http_ffi_client_op_t *op, ngx_msec_t idle_timeout, ngx_uint_t pool_size, u_char *err, size_t *errlen); diff --git a/src/ngx_http_ffi_client_request.c b/src/ngx_http_ffi_client_request.c index 0ee73ab..47aa7bb 100644 --- a/src/ngx_http_ffi_client_request.c +++ b/src/ngx_http_ffi_client_request.c @@ -2014,6 +2014,7 @@ ngx_http_ffi_client_op_reset(ngx_http_ffi_client_op_t *op) { u_char *rbuf; size_t rbuf_size; + ngx_uint_t generation; ngx_msec_t connect_timeout, send_timeout, read_timeout; ngx_http_request_t *r; @@ -2025,6 +2026,7 @@ ngx_http_ffi_client_op_reset(ngx_http_ffi_client_op_t *op) r = op->r; rbuf = op->rbuf; rbuf_size = op->rbuf_size; + generation = op->generation; connect_timeout = op->connect_timeout; send_timeout = op->send_timeout; read_timeout = op->read_timeout; @@ -2035,6 +2037,9 @@ ngx_http_ffi_client_op_reset(ngx_http_ffi_client_op_t *op) op->stateful = 1; op->rbuf = rbuf; op->rbuf_size = rbuf_size; + /* carried over, never restarted: a reader from the previous connection must + * stay distinguishable from the responses of the next one */ + op->generation = generation; op->connect_timeout = connect_timeout; op->send_timeout = send_timeout; op->read_timeout = read_timeout; @@ -2378,6 +2383,8 @@ ngx_http_ffi_client_ffi_send_request(ngx_http_request_t *r, op->body_offset = 0; 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; op->pending = NGX_HTTP_FFI_CLIENT_PENDING_REQUEST; op->state = NGX_HTTP_FFI_CLIENT_WRITING; @@ -2398,8 +2405,8 @@ ngx_http_ffi_client_ffi_send_request(ngx_http_request_t *r, */ 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; @@ -2407,6 +2414,12 @@ ngx_http_ffi_client_ffi_read_body(ngx_http_request_t *r, 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"); + } + if (op->pending != NGX_HTTP_FFI_CLIENT_PENDING_NONE) { return ngx_http_ffi_client_set_err(err, errlen, "busy"); } @@ -2475,7 +2488,7 @@ ngx_http_ffi_client_ffi_read_body(ngx_http_request_t *r, */ void ngx_http_ffi_client_ffi_get_trailers(ngx_http_ffi_client_op_t *op, - ngx_http_ffi_client_ffi_resp_t *resp) + ngx_uint_t generation, ngx_http_ffi_client_ffi_resp_t *resp) { if (op == NULL || resp == NULL) { return; @@ -2484,6 +2497,11 @@ ngx_http_ffi_client_ffi_get_trailers(ngx_http_ffi_client_op_t *op, resp->trailers = NULL; resp->trailers_len = 0; + /* trailers belong to one response, same as its body */ + if (generation != op->generation) { + return; + } + if (!op->parser.done) { return; } @@ -2492,6 +2510,15 @@ ngx_http_ffi_client_ffi_get_trailers(ngx_http_ffi_client_op_t *op, } +/* The response the op is on right now. A caller captures it with the response + * it just got and hands it back on every read of that response's body. */ +ngx_uint_t +ngx_http_ffi_client_ffi_get_generation(ngx_http_ffi_client_op_t *op) +{ + return op == NULL ? 0 : op->generation; +} + + ngx_uint_t ngx_http_ffi_client_ffi_get_reused_times(ngx_http_ffi_client_op_t *op) { @@ -2548,6 +2575,9 @@ ngx_http_ffi_client_ffi_set_keepalive(ngx_http_request_t *r, op->connected = 0; op->request_sent = 0; + /* the connection is the pool's again: readers of the response it carried + * must not touch whatever the next borrower reads on it */ + op->generation++; ngx_http_ffi_client_clear_cleanup(op); return NGX_OK; @@ -2569,6 +2599,8 @@ ngx_http_ffi_client_ffi_close(ngx_http_ffi_client_op_t *op) op->conn_broken = 0; op->request_sent = 0; op->close_requested = 0; + /* the response is gone with the connection */ + op->generation++; ngx_http_ffi_client_clear_cleanup(op); } diff --git a/t/016-body-reader-generation.t b/t/016-body-reader-generation.t new file mode 100644 index 0000000..951652f --- /dev/null +++ b/t/016-body-reader-generation.t @@ -0,0 +1,325 @@ +use Test::Nginx::Socket -Base; + +repeat_each(1); +plan tests => repeat_each() * (blocks() * 3); + +our $HttpConfig = qq{ + lua_package_path "$ENV{TEST_NGINX_LUA_PACKAGE_PATH};;"; +}; + +no_long_string(); +run_tests(); + +__DATA__ + +=== TEST 1: a reader from an earlier response never reads a later one +--- http_config eval: $::HttpConfig +--- config + location /a { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("AAA") + } + } + + location /b { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("BBB") + } + } + + location /t { + content_by_lua_block { + local http = require "resty.ngx_http_ffi_client" + local httpc = http.new() + + assert(httpc:connect({ + host = "127.0.0.1", + port = ngx.var.server_port, + })) + + local res1 = assert(httpc:request({ + path = "/a", + preread_body = true, + })) + ngx.say("first=", res1.body) + + local res2 = assert(httpc:request({ path = "/b" })) + + local chunk, err = res1.body_reader() + ngx.say("stale=", tostring(chunk), " ", tostring(err)) + + -- the refused read left the second response untouched + ngx.say("second=", assert(res2:read_body())) + httpc:close() + } + } +--- request +GET /t +--- response_body +first=AAA +stale=nil stale body reader +second=BBB +--- no_error_log +[error] + + + +=== TEST 2: read_body on a stale response reports the error +--- http_config eval: $::HttpConfig +--- config + location /a { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("AAA") + } + } + + location /b { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("BBB") + } + } + + location /t { + content_by_lua_block { + local http = require "resty.ngx_http_ffi_client" + local httpc = http.new() + + assert(httpc:connect({ + host = "127.0.0.1", + port = ngx.var.server_port, + })) + + local res1 = assert(httpc:request({ + path = "/a", + preread_body = true, + })) + -- drop the body the fast path handed over, so read_body has to + -- reach for the reader + res1.body = nil + + assert(httpc:request({ path = "/b" })) + + local body, err = res1:read_body() + ngx.say("body=", tostring(body)) + ngx.say("err=", tostring(err)) + httpc:close() + } + } +--- request +GET /t +--- response_body +body=nil +err=stale body reader +--- no_error_log +[error] + + + +=== TEST 3: a reader kept across set_keepalive is refused +--- http_config eval: $::HttpConfig +--- config + location /a { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("AAA") + } + } + + location /t { + content_by_lua_block { + local http = require "resty.ngx_http_ffi_client" + local httpc = http.new() + + assert(httpc:connect({ + host = "127.0.0.1", + port = ngx.var.server_port, + pool = "generation-keepalive", + })) + + local res = assert(httpc:request({ + path = "/a", + preread_body = true, + })) + + assert(httpc:set_keepalive()) + + local chunk, err = res.body_reader() + ngx.say("stale=", tostring(chunk), " ", tostring(err)) + } + } +--- request +GET /t +--- response_body +stale=nil stale body reader +--- no_error_log +[error] + + + +=== TEST 4: a reader kept across close is refused +--- http_config eval: $::HttpConfig +--- config + location /a { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("AAA") + } + } + + location /t { + content_by_lua_block { + local http = require "resty.ngx_http_ffi_client" + local httpc = http.new() + + assert(httpc:connect({ + host = "127.0.0.1", + port = ngx.var.server_port, + })) + + local res = assert(httpc:request({ + path = "/a", + preread_body = true, + })) + + httpc:close() + + local chunk, err = res.body_reader() + ngx.say("stale=", tostring(chunk), " ", tostring(err)) + } + } +--- request +GET /t +--- response_body +stale=nil stale body reader +--- no_error_log +[error] + + + +=== TEST 5: a reader kept across a reconnect is refused +--- http_config eval: $::HttpConfig +--- config + location /a { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("AAA") + } + } + + location /b { + content_by_lua_block { + ngx.header["Content-Length"] = 3 + ngx.print("BBB") + } + } + + location /t { + content_by_lua_block { + local http = require "resty.ngx_http_ffi_client" + local httpc = http.new() + + local opts = { + host = "127.0.0.1", + port = ngx.var.server_port, + } + + assert(httpc:connect(opts)) + + local res1 = assert(httpc:request({ + path = "/a", + preread_body = true, + })) + + httpc:close() + assert(httpc:connect(opts)) + + local res2 = assert(httpc:request({ path = "/b" })) + + local chunk, err = res1.body_reader() + ngx.say("stale=", tostring(chunk), " ", tostring(err)) + ngx.say("second=", assert(res2:read_body())) + httpc:close() + } + } +--- request +GET /t +--- response_body +stale=nil stale body reader +second=BBB +--- no_error_log +[error] + + + +=== TEST 6: the happy path is unchanged, one reader per response +--- http_config eval: $::HttpConfig +--- config + location /a { + content_by_lua_block { + local body = ("a"):rep(20000) + ngx.header["Content-Length"] = #body + ngx.print(body) + } + } + + location /b { + content_by_lua_block { + local body = ("b"):rep(20000) + ngx.header["Content-Length"] = #body + ngx.print(body) + } + } + + location /t { + content_by_lua_block { + local http = require "resty.ngx_http_ffi_client" + local httpc = http.new() + + assert(httpc:connect({ + host = "127.0.0.1", + port = ngx.var.server_port, + })) + + local function drain(res) + local parts = {} + while true do + local chunk, err = res.body_reader() + if err then + return nil, err + end + + if not chunk then + break + end + + parts[#parts + 1] = chunk + end + + return table.concat(parts) + end + + local res1 = assert(httpc:request({ path = "/a" })) + local first = assert(drain(res1)) + + local res2 = assert(httpc:request({ path = "/b" })) + local second = assert(drain(res2)) + + ngx.say("first=", first == ("a"):rep(20000)) + ngx.say("second=", second == ("b"):rep(20000)) + -- a drained reader stays done rather than reaching for more + ngx.say("again=", tostring(res1.body_reader())) + httpc:close() + } + } +--- request +GET /t +--- response_body +first=true +second=true +again=nil +--- no_error_log +[error]