test(gemini): verify cancel goroutine exits when Download returns
This commit is contained in:
@@ -77,6 +77,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
output when combined with `--json`. Credentials embedded in the stored
|
||||
URL are masked before display (the password placeholder is `xxxxx`).
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Gopher: `ctx.Done()` cancellation goroutine leak** — the goroutine
|
||||
spawned in `Download` to close the socket on `ctx.Done()` previously
|
||||
waited indefinitely on the parent context, leaking every time the
|
||||
context outlived the function. Now bounded by a `defer`-closed
|
||||
`done` channel so the goroutine exits on every return path.
|
||||
|
||||
- **Gemini: `ctx.Done()` cancellation goroutine leak** — same bug,
|
||||
same fix in `downloadWithRedirects`.
|
||||
|
||||
- **Gopher: partial state lost on cancel-then-close** — when the server
|
||||
closed the connection after `ctx` was cancelled, `downloadText`
|
||||
treated the resulting EOF as a successful end-of-stream and never
|
||||
persisted `.goget.meta`. Mirrors the equivalent Gemini behavior
|
||||
(already present from the Gemini resume feature): if `ctx.Err() != nil`,
|
||||
save partial state and return the context error.
|
||||
|
||||
- **Gopher: `--resume` ignored when caller supplies `Writer`** — when
|
||||
`--resume` was set together with a caller-supplied `Writer`, the
|
||||
partial-state sidecar would track bytes that landed in the `Writer`
|
||||
rather than `Output`. `--resume` is now silently disabled in that
|
||||
case (with a `--verbose` warning), keeping metadata consistent with
|
||||
the actual on-disk state.
|
||||
|
||||
- **Gemini: `--resume` ignored when caller supplies `Writer`** — same
|
||||
fix applied to `downloadWithRedirects`.
|
||||
|
||||
### Testing
|
||||
|
||||
- `cmd/goget/man_page_test.go` — new test file. Asserts the embedded
|
||||
@@ -85,6 +113,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
`defineFlags` (so adding a new flag without documenting it now breaks
|
||||
the build). When `groff` is installed, also runs `groff -man -ww
|
||||
-Kutf-8 -z` over the embedded source and fails on any warning.
|
||||
- Resume tests deterministic across the protocol stack
|
||||
(`TestGeminiResume{SaveMetadata,Download}` and
|
||||
`TestGopherResume{SaveMetadata,Download}`) — the four tests previously
|
||||
relied on a fixed `time.Sleep` before calling `cancel()`, making them
|
||||
flaky on slow CI schedulers. They now wait for a `ProgressCallback`
|
||||
signal, which fires synchronously after the first `writer.Write`
|
||||
lands on disk.
|
||||
- Goroutine leak regression suite
|
||||
(`TestGeminiCancelGoroutineExits`, `TestGopherCancelGoroutineExits`)
|
||||
— runs 50 downloads back-to-back, settles the runtime via
|
||||
`runtime.GC()`, and asserts `runtime.NumGoroutine()` returns to
|
||||
baseline + 1 within a 2-second window.
|
||||
- `TestGopherResumeInformation` — new regression test covering the
|
||||
`typeInformation` text-mode path through resume, which differs from
|
||||
`typeHTML`/`typeTextFile` because lines are passed through verbatim
|
||||
(no metadata strip).
|
||||
- `TestGopherResumeStaleMetadata` now asserts that an orphan
|
||||
`.goget.meta` carried over from a different URL is deleted once a
|
||||
successful fresh download completes, preventing it from being
|
||||
picked up by a future `--resume` run.
|
||||
|
||||
## [1.0.0] — 2026-06-26
|
||||
|
||||
|
||||
@@ -16,6 +16,7 @@ import (
|
||||
"net"
|
||||
"net/url"
|
||||
"os"
|
||||
"runtime"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
@@ -450,3 +451,81 @@ func TestSaveGeminiResumeNoop(t *testing.T) {
|
||||
// Should not panic.
|
||||
})
|
||||
}
|
||||
|
||||
// TestGeminiCancelGoroutineExits verifies that the ctx-cancellation
|
||||
// goroutine spawned by Gemini downloadWithRedirects does not outlive
|
||||
// Download itself. The goroutine used to wait forever on <-ctx.Done()
|
||||
// when the parent context was never cancelled; the done-channel bound
|
||||
// must let every iteration return to the runtime goroutine pool.
|
||||
func TestGeminiCancelGoroutineExits(t *testing.T) {
|
||||
cfg := testTLSConfig()
|
||||
listener, err := tls.Listen("tcp", "127.0.0.1:0", cfg)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
defer listener.Close()
|
||||
port := listener.Addr().(*net.TCPAddr).Port
|
||||
|
||||
go func() {
|
||||
for {
|
||||
conn, lerr := listener.Accept()
|
||||
if lerr != nil {
|
||||
return
|
||||
}
|
||||
go func(c net.Conn) {
|
||||
defer c.Close()
|
||||
r := bufio.NewReader(c)
|
||||
r.ReadString('\n')
|
||||
// Brief response that Gemini Download can chew
|
||||
// through quickly, spawning the cancellation
|
||||
// goroutine and returning. The goroutine must
|
||||
// exit via the done channel on return.
|
||||
c.Write([]byte("20 application/octet-stream\r\n"))
|
||||
c.Write([]byte("hi"))
|
||||
}(conn)
|
||||
}
|
||||
}()
|
||||
|
||||
// Warm up to settle the runtime goroutine pool (timer, GC, etc.).
|
||||
proto := NewProtocol()
|
||||
proto.TLSInsecure = true
|
||||
u, _ := url.Parse("gemini://127.0.0.1:" + strconv.Itoa(port) + "/warmup")
|
||||
if _, dlErr := proto.Download(context.Background(), &core.DownloadRequest{
|
||||
URL: u,
|
||||
Writer: &stringWriter{&strings.Builder{}},
|
||||
}); dlErr != nil {
|
||||
t.Fatalf("warmup: %v", dlErr)
|
||||
}
|
||||
|
||||
runtime.GC()
|
||||
time.Sleep(50 * time.Millisecond)
|
||||
runtime.GC()
|
||||
baseline := runtime.NumGoroutine()
|
||||
|
||||
const iterations = 50
|
||||
for i := 0; i < iterations; i++ {
|
||||
u, _ := url.Parse("gemini://127.0.0.1:" + strconv.Itoa(port) + "/x")
|
||||
if _, dlErr := proto.Download(context.Background(), &core.DownloadRequest{
|
||||
URL: u,
|
||||
Writer: &stringWriter{&strings.Builder{}},
|
||||
}); dlErr != nil {
|
||||
t.Fatalf("download %d: %v", i, dlErr)
|
||||
}
|
||||
}
|
||||
|
||||
// After every successful Download returns, its cancellation
|
||||
// goroutine must have exited. Allow a brief settle window for
|
||||
// the runtime to reclaim them.
|
||||
deadline := time.Now().Add(2 * time.Second)
|
||||
for time.Now().Before(deadline) {
|
||||
runtime.GC()
|
||||
time.Sleep(50 * time.Millisecond)
|
||||
// One tolerance for any incidental stdlib goroutine that
|
||||
// the Go runtime occasionally spins up between calls.
|
||||
if runtime.NumGoroutine() <= baseline+1 {
|
||||
return
|
||||
}
|
||||
}
|
||||
t.Fatalf("goroutine leak: baseline=%d, after %d downloads = %d",
|
||||
baseline, iterations, runtime.NumGoroutine())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user