api7 / api7/ngx_http_ffi_client

set_timeout/set_timeouts: validate inputs and define the connect-stage timeout budget

Open
#24 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement P2 priority-normal
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 nginx resolver_timeout directive, not by connect_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; define 0 (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 the lua-resty-http strings ai-transport maps 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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.