shreemaan-abhishek commented on PR #13232:
URL: https://github.com/apache/apisix/pull/13232#issuecomment-4301494151
### `ctx.tracing = nil` in `release()` is a no-op on a request-phase ctx
@hachi029 already demonstrated this empirically. The per-request `ngx.ctx`
in an HTTP phase is created as a fresh empty table whose `__index` metatable
points at the **connection-scoped** SSL-phase ctx. On the same TLS connection
this means three distinct Lua tables are in play:
- The **SSL-phase ctx table**: lives on the TLS connection, populated once
during the handshake, shared with every HTTP request on that connection via the
metatable link below.
- **Request 1's per-request ctx table**: a fresh empty table created on
first `ngx.ctx` access in request 1, whose metatable is the SSL-phase ctx table.
- **Request 2's per-request ctx table**: another fresh empty table created
on first `ngx.ctx` access in request 2, whose metatable is also the SSL-phase
ctx table.
`tracer.start()` runs in `ssl_client_hello_phase` (apisix/init.lua:209),
where `ctx` is the SSL-phase ctx table directly, so it writes `tracing` into
the SSL-phase ctx table. In every later HTTP phase, reading `ctx.tracing` on a
per-request ctx finds no own field called `tracing` and falls through `__index`
to the SSL-phase ctx table's `tracing` field. Writing `ctx.tracing = nil` on a
per-request ctx only affects that per-request ctx; the SSL-phase ctx table has
no `__newindex` metamethod, so its `tracing` field is untouched and the next
read on any per-request ctx still returns the same tracing table.
This matches @hachi029's repro: the logged table address `0x7ff77b5a1060` is
identical in the SSL phase, both concurrent HTTP/2 streams, and the log phase,
and `ngx.ctx.tracing = nil` followed by a re-read returns the same table.
So line 88 (`ctx.tracing = nil`) is dead code under the metatable
inheritance that OpenResty actually uses. The crash is prevented purely by the
`not tracing.spans` guard on line 42, and it only happens to work because
`tablepool.fetch` returns the just-released tracing table back (LIFO),
effectively re-fusing it onto the SSL-phase ctx's `tracing` field.
### Proposal: sever the inheritance with `rawget` / `rawset`
Both issues disappear if `tracer.start` and `tracer.release` operate on the
per-request ctx directly, bypassing the metatable:
```lua
function _M.start(ctx, name, kind)
...
local tracing = rawget(ctx, "tracing")
if not tracing then
tracing = tablepool.fetch("tracing", 0, 8)
tracing.spans = tablepool.fetch("tracing_spans", 20, 0)
rawset(ctx, "tracing", tracing)
end
...
end
function _M.release(ctx)
local tracing = rawget(ctx, "tracing")
if not tracing then
return
end
for _, sp in ipairs(tracing.spans) do
sp:release()
end
tablepool.release("tracing_spans", tracing.spans)
tablepool.release("tracing", tracing)
rawset(ctx, "tracing", nil)
end
```
Why this works:
- In the SSL phase, `ctx` is the SSL-phase ctx table directly; `rawget` on
it is identical to the regular field read. The SSL-handshake span continues to
be recorded on the SSL-phase ctx table's `tracing` field as before.
- In every HTTP phase, `rawget(ctx, "tracing")` returns `nil` on first
access because the per-request ctx has no own `tracing` key and `rawget` does
not follow `__index`. Each request initialises its own fresh tracing table on
its own per-request ctx. No cross-stream sharing, no stale-pointer crash, no
double-release.
- The `not tracing.spans` defensive check is no longer needed `rawget`
guarantees each phase only ever sees its own tracing table, which is either
`nil` or fully initialised.
### CAVEAT: Release the SSL-phase tracing table explicitly
With `rawget` in place, the tracing table on the SSL-phase ctx (set during
`ssl_client_hello_phase`) is no longer picked up or released by any HTTP
request phase, so it would leak for the lifetime of the TLS connection. It
should be released at the end of `ssl_client_hello_phase` in `apisix/init.lua`,
immediately after `span:finish(ngx_ctx)`:
```lua
span:finish(ngx_ctx)
tracer.release(ngx_ctx)
```
This is, in effect, what's happening implicitly today the SSL-phase tracing
table is being released by request 1's log phase, which is exactly the coupling
that causes #13200. Making it explicit and SSL-scoped is the correct shape.
One consequence: the SSL-handshake span becomes its own standalone trace
rather than being stitched into the HTTP request's trace. Stitching across SSL
and HTTP requires propagating only the *span context* (traceparent IDs), not
the live tracing object a separate, larger change.
TEST 4 currently only asserts the response body, which isn't sensitive to
either span mixing or log-phase failures. A useful addition would be to assert,
at the end of each request, that `ngx.ctx.tracing.spans` contains only the
spans belonging to that request, i.e. no cross-stream contamination.
--
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]
