fix(config,std): make the -1 'no timeout' sentinel idempotent and map it onto net/http's own encoding - #606
Conversation
…ntinel survives (celeris#594)
Mechanism: WithDefaults carries "disabled" as 0 because that is what every
consumer reads as off (`> 0` guards in the iouring/epoll loops, http.Server's
own `d > 0` checks in std), but 0 on the way IN means "give me the default".
The mapping was therefore not idempotent, and normalisation runs at least
twice on every start: Server.doPrepare (server.go:569) normalises, then each
engine's New normalises again (iouring/engine.go:73, epoll/engine.go:58,
std/engine.go:54), three times through adaptive/engine.go:213. A documented
ReadHeaderTimeout=-1 became -1 -> 0 -> 10s, and the same for ReadTimeout,
WriteTimeout, IdleTimeout and MaxRequestBodySize (-1 "unlimited" -> 100 MB).
Fix: an unexported `defaulted` marker on resource.Config records that the
sentinel fields have been resolved; resolveTimeout applies the 0 -> default
branch only on the first pass and always maps a negative to the disabled
encoding. Every other default is unchanged and was already idempotent. No
consumer changes, no negative duration can reach net/http, no per-request hot
path touched (WithDefaults is construction-time only) and sizeof(Config) is
216 bytes before and after -- the bool lands in existing padding.
Measured (docker golang:1.27, --cpus 4, seccomp=unconfined, memlock 128 MB):
new tests, fixed tree: resource, engine/std, engine/epoll, engine/iouring,
adaptive all ok, 0 skips (the io_uring engine was
really constructed; the test only skips on a
genuine "io_uring not available").
negative control (same tests, resource/config.go reverted to origin/main):
resource pass 2: ReadHeaderTimeout = 10s, want 0s (input -1ns)
pass 2: ReadTimeout/WriteTimeout = 1m0s; IdleTimeout = 10m0s
pass 2: MaxRequestBodySize = 104857600, want 0
std cfg.ReadHeaderTimeout = 10s and http.Server.ReadHeaderTimeout
= 10s, want 0s (plus the other three)
epoll adaptiveTimeoutMs with ReadHeaderTimeout disabled:
idle=25ms detached=25ms, want 400/50
iouring adaptiveTimeout with ReadHeaderTimeout disabled:
idle=25ms detached=25ms, want 100ms/50ms <- celeris#584: the
sweep gate was pinned at 0x1F regardless of detachedCount
adaptive sub-engine re-normalisation changed the timeouts:
1m0s/10s/1m0s/10m0s, want 0s/0s/0s/0s
Suites (container, -count=1): ./ ./engine/... ./resource/... ok.
adaptive's TestRampH1Sync/TestRampH1Async fail identically with this change
reverted (same "phase 1 (low load): conns not on epoll: epoll=0 io_uring=128"),
so they are a pre-existing container failure, not a regression.
…ris#594)
Making WithDefaults idempotent got the -1 "no timeout" sentinel as far as
resource.Config, but not as far as the wire on std. celeris carries "disabled"
as 0 because that is what every consumer reads as off; net/http is not uniform
about 0 (go1.27 src/net/http/server.go, read, not guessed):
ReadTimeout 0 or negative => no timeout (:1039, `d > 0`)
WriteTimeout 0 or negative => no timeout (:1042, `d > 0`)
ReadHeaderTimeout 0 => FALL BACK TO ReadTimeout (:3752, used :2038/:2177)
IdleTimeout 0 => FALL BACK TO ReadTimeout (:3745, used :2163)
So ReadHeaderTimeout=-1 next to the 60s ReadTimeout default reached
http.Server as 0 and net/http quietly re-armed it at 60s; likewise IdleTimeout.
The issue asks for "-1 reaches every engine as disabled" and std was the engine
where it did not.
Fix: a stdTimeout helper maps any non-positive value to -1 when the
http.Server literal is built, with the net/http semantics named in the comment.
Every consumption site above guards with `d > 0`, so a negative is inert
everywhere and never becomes a deadline; the same encoding is used on all four
fields rather than relying on two of them happening to accept 0. No behaviour
change for positive values. resource/config.go is untouched by this commit.
Tests: the std table test now carries per-field inputs and two mixed cases
(header+idle disabled while ReadTimeout stays enabled, at the default and at an
explicit 3s), and asserts disabled TWICE per field -- cfg.<field> == 0 and
"net/http enforces nothing", the latter computed by netHTTPEffective, which
replicates the two fallbacks. Plus two wire tests on a real loopback socket:
a slowloris client dribbling its header block byte-by-byte, and a keep-alive
connection left idle.
Measured (darwin/arm64 and docker golang:1.27 --cpus 4 seccomp=unconfined
memlock 128MB, identical results):
fixed tree ./engine/... ./adaptive/... ./resource/... -run Sentinel
all PASS, 0 skips. std wire: explicit RHT=300ms kills the
dribbler at 301ms with 400 Bad Request; RHT=-1 with
ReadTimeout=300ms serves 200 OK after a 10.83s dribble;
IdleTimeout=-1 with ReadTimeout=300ms serves a second
request after 1.2s idle.
negative control same tests, resource/config.go reverted to origin/main:
FAIL. Table: all-disabled/via-doPrepare,
mixed-with-default-read/via-doPrepare and
mixed-with-explicit-read/via-doPrepare. Wire:
sentinel-disables killed at 10.001s -- the 10s
ReadHeaderTimeout default reinstated over the sentinel,
which is why the dribble is 11s.
negative control same tests, resource/config.go fixed but engine/std
(this gap) reverted to af5a280: FAIL. Table: 6 subtests, e.g.
"ReadHeaderTimeout disabled in config but net/http still
enforces 1m0s (http.Server.ReadHeaderTimeout=0s,
ReadTimeout=1m0s)" and the same for IdleTimeout. Wire:
sentinel-disables killed at 301ms (the ReadTimeout
fallback), idle conn EOF on the second request.
Suites (container, -count=1): ./ ./engine/... ./resource/... all ok.
golangci-lint run ./... and GOOS=linux: 0 issues; go vet clean both GOOS.
Known limit, not fixed here: TestIdleTimeoutSentinelOnTheWire discriminates
the std gap but not origin/main, because origin/main reinstates the 600s
IdleTimeout default and no bounded test can outlive it. The table test covers
IdleTimeout against origin/main.
ab2536c to
c369c65
Compare
…om ReadHeaderTimeout That is net/http's rule, never celeris's, and it contradicted the sentence nine lines below it in the same block (0 asks for the default, -1 disables).
c369c65 to
454d4b6
Compare
|
The failing check is celeris#610, not this change. |
|
Third unrelated CI failure on this branch, third time it is not this change. Running tally for this branch, which touches only |
Fixes #594 (found while measuring #584). The rig that found the defect judges the fix, the negative control was run, and an adversarial reviewer re-read the pushed diff rather than the report.
Reviewer verdict
Finding closed: True. Mergeable: True.
I read the pushed diff (origin/fix/594-timeout-sentinel, 6188b4b + af5a280, 7 files, 2 production files) rather than trusting the report, and re-ran the controls myself in a throwaway
git archivecopy (nothing in the repo touched).FINDING 1 — celeris's "disabled" encoding (0) does not mean disabled to net/http on ReadHeaderTimeout/IdleTimeout. CLOSED. engine/std/engine.go now routes all four http.Server fields through
stdTimeout(d)(d <= 0 -> -1). I verified the semantics claim against the actual toolchain source rather than the comment: /opt/homebrew/Cellar/go/1.27.0/libexec/src/net/http/server.go:1039 and :1042 guard ReadTimeout/WriteTimeout withd > 0, and :3745-3756 are the two fallbacks (if s.IdleTimeout != 0 { return s.IdleTimeout }; return s.ReadTimeout, same shape for ReadHeaderTimeout). So the documented mapping is exact, and -1 is safe on all four because every consumption site is a> 0guard. The only other reader of these fields istlsHandshakeTimeout(server.go:969, "minimum of any POSITIVE"), which skips <= 0 exactly as it skipped 0 before — and engine/std never serves TLS (no TLSConfig/ServeTLS anywhere in the package;configureHTTP2/h2IdleTimeoutis unreachable becauseshouldConfigureHTTP2ForServeneeds a TLSConfig, and the h2c path uses its ownhttp2.Server). No behaviour change outside the intended one.FINDING 2 — the table test used one value for all four fields and asserted only the celeris encoding. CLOSED. engine/std/timeout_sentinel_test.go is now per-field, includes the two mixed cases (
{0,-1,0,-1}and{3s,-1,3s,-1}: header+idle disabled while ReadTimeout stays enabled — the only shape that exposes the fallback), runs each case both raw and via-doPrepare, and asserts three things per field:e.cfg.X, the rawhttp.Server.X(must be strictly negative when disabled), andnetHTTPEffective(net/http's own resolution, transcribed from the source lines above). Plus two real-socket tests.RE-VERIFICATION NUMBERS — reproduced, not taken on faith:
go test ./resource/ ./engine/std/both ok; the wire tests logexplicit-header-timeout-kills served=false firstResultAfter=302ms response="HTTP/1.1 400 Bad Request"andsentinel-disables served=true firstResultAfter=10.894s(i.e. it outlived the 10s default), idle test serves a 200 after 1.2s idle with ReadTimeout=300ms./via-doPreparesubtests fail (all-disabled, mixed-with-default-read, mixed-with-explicit-read) and the header wire test dies at 10.002s — matches the report's 10.001s.second request after idle: read: EOF), so the idle half has its own negative control too.Both controls therefore isolate the two halves of the bug independently, which is the property that was missing before.
NOTHING NEW BROKEN, checked specifically:
resolveTimeoutandstdTimeoutrun only in WithDefaults and the std constructor.unsafe.Sizeof(resource.Config{})is 216 bytes on both main and the branch — the new unexporteddefaulted boollands in existing padding, so the per-worker/per-loop Config copies cost nothing.> 0guard (engine/iouring/worker.go:1034, :1357, :4356-4362; engine/epoll/loop.go:297, :649, :2365, :2473-2479) or an== 0no-op (internal/conn/h1.go:64), so propagating a real 0 is what those comments already promise. I hand-checked the two linux assertions I cannot run: with emptyIters=200/listenFD=3adaptiveTimeoutreally returns 100ms idle / 50ms detached with the header timeout off and 25ms/25ms with it on, and epoll'sadaptiveTimeoutMs(100)with consecutiveEmpty=200 really returns 400/50 vs 25/25 — the test expectations match the source.defaultedmarker cannot leak a stale "already normalised" into a fresh config:Config.toResourceConfig(config.go:247) builds a literal every time, it is the only resource.Config literal in non-test code, anddoPrepare's configureFn only sets Listener. Config has func fields so nothing compares it with==, and there is no reflect.DeepEqual on it anywhere.go vetclean on darwin, andGOOS=linux CGO_ENABLED=0 go vet ./engine/epoll/ ./engine/iouring/ ./adaptive/is clean, so the three linux test files compile and have no duplicate symbols. Fullgo test .,./resource/,./engine/std/all pass.Residuals the reviewer recorded
Nothing blocking. Three residuals, all minor:
Self-contradicting doc comment the commit touched but did not clean. resource/config.go:46 still says of ReadHeaderTimeout "Zero falls back to ReadTimeout." — that is net/http's rule, never celeris's — while the line the commit appended at the bottom of the same block says "0 asks for the default (10s); -1 disables it (0 after WithDefaults)." The stale sentence predates this branch, but it now sits nine lines above its own contradiction inside a comment this commit edited. One-line deletion.
Validate() asymmetry (pre-existing, not a regression). resource/config.go:132-139 bounds ReadTimeout/WriteTimeout/IdleTimeout at >= -1 but has no such check for ReadHeaderTimeout, so ReadHeaderTimeout=-5 is silently accepted as "disabled" while ReadTimeout=-5 is an error. Unchanged by this branch (both before and after, WithDefaults collapses any negative before Validate ever sees it, so even the three checks that exist are dead for engine-constructed configs — only doPrepare's pre-normalised path can trip them).
Small public-API semantics change worth a release note: resource is an exported package, so an external caller who does
cfg := resource.Config{...}.WithDefaults()and then setscfg.ReadTimeout = 0before handing it to an engine constructor now gets "disabled" where they previously got the 60s default. That is the deliberate point of thedefaultedmarker and is documented on the field, but it is a behaviour change beyond the four timeouts named in Config timeouts: the documented -1 'no timeout' sentinel is normalised twice and becomes the 10 s default (ReadHeaderTimeout, ReadTimeout, WriteTimeout) #594 (MaxRequestBodySize moves the same way: a double-normalised unlimited now stays unlimited instead of snapping back to 100 MB).Also note the linux-only tests (epoll, iouring, adaptive) were type-checked here by cross-GOOS vet but not executed — they need the cluster.