mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 22:37:03 +00:00
xarmian
524469b7a3
Closes BUG-3008. Closes BUG-2949, which filed this failure on 09-07 with the
mechanism explicitly undiagnosed and stopped in the right place: it refuted the
obvious candidate, noted a repo-wide grep found no other caller, and listed
three hypotheses. This is hypothesis (1), and the caller is the standard
library rather than this repo.
httptest.Server.Close() calls CloseIdleConnections() on the PROCESS-WIDE
http.DefaultTransport — deliberately, and the standard library says so in its
own comment, calling it "not part of httptest.Server's correctness". So in a
package where many tests each stand up a server, every `defer ts.Close()`
mutates state every other test's requests depend on. CI observed a request
failing in one test while the server that closed belonged to another:
client_stream_identity_test.go:135: request not sendable:
Get "http://127.0.0.1:45521/api/v1/events/stream?armed=true":
net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called
SCOPE is every test request in internal/server, not just the parallel ones.
Three revisions were needed to get the class right, and each earlier boundary
was wrong in a way worth recording:
- Direct call sites in test bodies missed SHARED HELPERS. connectSSE,
apiRequest and readRawSSEFramesAuthed sent through the default transport
and have parallel callers; connectSSE holds its request open for a whole
test. A helper cannot know which of its callers is exposed, so it no
longer tries to.
- "Serial tests are safe" is true but is the wrong boundary. Serial-ness is
one t.Parallel() away, and CONVE-2086 tells the next author to add exactly
that line.
- A nil Transport IS http.DefaultTransport. listen_serve_test.go's health
probe was exposed identically and appears in no grep for DefaultClient.
internal/cli is deliberately different: its tests drive the PRODUCTION client
from NewClientFromURL, which has a nil Transport by design, so those sends
cannot be isolated without changing the shipped CLI. There the boundary really
is parallel-vs-serial, and the two parallel tests that build their own requests
use srv.Client(). The asymmetry is written into isolatedTestClient's doc
comment rather than left to be inferred.
WHAT IS NOT CLAIMED: Transport.CloseIdleConnections documents that it "does not
interrupt any connections currently in use", and the idle pool is mutated under
idleMu. The interleaving by which errCloseIdleConns reached a caller's Do() is
NOT reconstructed, and the comment says so instead of inventing one. That
unknown is the argument for isolation over a narrower repair: a client with its
own transport is out of reach whichever window it was.
CONVE-2086 amended with the rule, so the convention that creates the exposure
also carries it.
14 files, all _test.go. Codex: four rounds, CLEAN on the fourth; rounds 1-3
each found a real defect in the class boundary or in the comment's claims about
net/http. Gates on 8dfd6ad3: go test ./... 0, go vet 0, gofmt clean, lint 0
issues; all 8 CI checks green.
61 lines
3.2 KiB
Go
61 lines
3.2 KiB
Go
package server
|
|
|
|
import "net/http"
|
|
|
|
// isolatedTestClient returns an HTTP client with a transport of its own.
|
|
// Outbound requests in this package's tests go through it — or through the
|
|
// httptest server's own `ts.Client()` where the test holds the server value —
|
|
// and never through `http.DefaultClient`, `http.Get`, or an `http.Client`
|
|
// literal with a nil Transport, which is `http.DefaultTransport` (BUG-3008).
|
|
//
|
|
// WHY THIS EXISTS. `httptest.Server.Close()` reaches into the PROCESS-WIDE
|
|
// default transport — the standard library says so in its own comment, calling
|
|
// it "not part of httptest.Server's correctness" and doing it to help out
|
|
// users who are on the standard transport:
|
|
//
|
|
// if t, ok := http.DefaultTransport.(closeIdleTransport); ok {
|
|
// t.CloseIdleConnections()
|
|
// }
|
|
//
|
|
// So in a package where many tests each stand up an httptest server, every
|
|
// `defer ts.Close()` mutates state that every other test's requests depend on.
|
|
// What CI observed (BUG-2949, then BUG-3008) is a request in one test failing
|
|
// with "transport connection broken: http: CloseIdleConnections called" while
|
|
// the server that closed belonged to a different test entirely.
|
|
//
|
|
// WHAT IS AND IS NOT CLAIMED HERE. Read out of `net/http/transport.go`,
|
|
// `Transport.CloseIdleConnections` closes each pooled HTTP/1 connection with
|
|
// `errCloseIdleConns` — the error in that message; HTTP/2 connections go
|
|
// through `h2transport.CloseIdleConnections()` separately. It sets
|
|
// `closeIdle`, so a connection going idle afterwards is closed rather than
|
|
// pooled when no request is waiting for one — a waiter is still handed it —
|
|
// until a later `queueForIdleConn` clears the flag (which it does not reach
|
|
// when `DisableKeepAlives` is set). It also cancels the dials in progress that
|
|
// have a `cancelCtx` and are not waiting.
|
|
//
|
|
// Its doc comment states that it "does not interrupt any connections currently
|
|
// in use", and the idle pool is mutated under `idleMu`, so the precise
|
|
// interleaving by which that error reached a caller's `Do()` is NOT
|
|
// reconstructed here.
|
|
//
|
|
// That unknown is the argument for isolation rather than a narrower repair: a
|
|
// client with its own transport is outside the reach of another test's
|
|
// teardown whichever window it was. Guessing at the window and fixing only
|
|
// that would leave the next seat to rediscover this from a worse position.
|
|
//
|
|
// It is deliberately not a shared package-level var: two tests holding one
|
|
// transport would reintroduce a smaller version of the same coupling.
|
|
//
|
|
// NOT A GENERAL RULE FOR EVERY PACKAGE. `internal/cli`'s tests drive the
|
|
// PRODUCTION client from `NewClientFromURL`, which has a nil Transport by
|
|
// design, so its sends cannot be isolated without changing what the shipped
|
|
// CLI does. There the boundary really is parallel-vs-serial: the two
|
|
// `t.Parallel()` tests that build their own requests use `srv.Client()`, and
|
|
// the rest are serial, which a parallel test's teardown cannot overlap.
|
|
func isolatedTestClient() *http.Client {
|
|
// No Timeout on purpose. These are SSE and long-poll requests; a timeout
|
|
// here would cancel the stream the test is measuring. A caller that wants
|
|
// one sets it on the returned client.
|
|
return &http.Client{Transport: &http.Transport{}}
|
|
}
|