api7 / api7/ngx_http_ffi_client
set_timeout/set_timeouts: validate inputs and define the connect-stage timeout budget
Nobody has claimed this yet.
- 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).
client:set_timeout(t) / client:set_timeouts(connect, send, read) are wired through but their semantics are underspecified and untested.
Gaps
1. No validation.
client.set_timeouts (lib/resty/ngx_http_ffi_client.lua:467) stores whatever it is given: self.connect_timeout = connect_timeout or self.connect_timeout. A non-number reaches the FFI call and raises a cdata conversion error from inside the library; a negative number is converted into ngx_msec_t (unsigned) and silently becomes a huge timeout. On the C side ngx_http_ffi_client_ffi_set_timeouts() (src/ngx_http_ffi_client_request.c:1817) ignores any value <= 0, so set_timeout(0) is a silent no-op rather than "no timeout" or an error.
2. The connect deadline is per-stage, not one budget, and DNS is outside it.
- DNS:
rctx->timeout = clcf->resolver_timeout(src/ngx_http_ffi_client_request.c:641) — bounded by the nginxresolver_timeoutdirective, not byconnect_timeout. - TCP connect:
ngx_add_timer(c->write, op->connect_timeout)(src/ngx_http_ffi_client_request.c:321). - TLS handshake: a fresh
ngx_add_timer(c->read, op->connect_timeout)(src/ngx_http_ffi_client_request.c:1484).
So a caller asking for a 1s connect timeout can wait resolver_timeout + 1s + 1s before connect() returns. Whether that is intended (per-stage budgets, matching cosocket) or a bug (one shared deadline) needs to be decided and documented — ai-transport maps timeouts to 504, so the wall-clock a caller can observe matters.
3. No stall tests.
There is no test that actually stalls a connect, a TLS handshake, a send, or a read and asserts the timeout fires within the configured budget with the expected error string.
Proposal
- Validate arguments in
set_timeouts: reject non-numbers and negatives with a clear error; define0(either "unlimited" or rejected, but not a silent no-op). - Decide and document the connect-stage model: one shared connect deadline covering DNS + TCP + TLS, or explicit per-stage budgets with DNS bounded by
resolver_timeout. - Add real stall tests for connect, TLS handshake, send, and read, asserting both the elapsed budget and the error string (
timeout/timed out, matching thelua-resty-httpstringsai-transportmaps to 504).
Acceptance
- Documented timeout contract in the README.
- Test::Nginx cases covering each stalled stage.
- Green on both parser backends.
Related: #16 (framing error paths). Part of #18.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with lib/resty/ngx_http_ffi_client.lua:467 and the referenced timeout paths in src/ngx_http_ffi_client_request.c, then review the README and existing Test::Nginx coverage. Define and document the timeout contract, add stalled connect, TLS, send, and read cases with elapsed-time and error assertions, and verify both parser backends are green.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, lua, nginx
- Domain
- backend-api-design, documentation, testing
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100