`fatal error: runtime.SetFinalizer: finalizer already set` — `clientRespStream.Close()` is not idempotent and double-Put's into the pool
- Dominant language
- Go
- Stars
- 7.4k
- Forks
- 643
- Avg merge
- 14h 5m
- Merged PRs (30d)
- 2
Description
### Describe the bug
When a streamed client response body (`WithResponseBodyStream(true)`) has its `Close()` / `CloseBodyStream()` called more than once, the whole process crashes with:
```
fatal error: runtime.SetFinalizer: finalizer already set
runtime.SetFinalizer.func2()
/usr/local/go/src/runtime/mfinal.go:540
github.com/cloudwego/hertz/pkg/protocol/http1/resp.convertClientRespStream(...)
.../hertz@v0.10.3/pkg/protocol/http1/resp/response.go:203
github.com/cloudwego/hertz/pkg/protocol/http1/resp.ReadRespBodyStream(...)
.../hertz@v0.10.3/pkg/protocol/http1/resp/response.go:245
github.com/cloudwego/hertz/pkg/protocol/http1.(*HostClient).doNonNilReqResp(...)
.../hertz@v0.10.3/pkg/protocol/http1/client.go:726
```
### Root cause
`clientRespStream` is pooled via `sync.Pool`. `Close()` clears the finalizer and unconditionally returns the object to the pool, but there is **no guard against being called twice**:
```go
func (c *clientRespStream) Close() (err error) {
c.mu.Lock()
defer c.mu.Unlock()
runtime.SetFinalizer(c, nil)
err = ext.ReleaseBodyStream(c.r)
if c.closeCallback != nil {
...
err = c.closeCallback(err != nil)
}
c.r = nil
c.closeCallback = nil
clientRespStreamPool.Put(c) // <-- runs on every call
return
}
```
The `mu` mutex only serializes the two calls; it does not make `Close()` idempotent. If `Close()` (or `Response.CloseBodyStream()`) is invoked twice on the same stream, `clientRespStreamPool.Put(c)` enqueues the **same pointer into the pool twice**. A later `convertClientRespStream()` then `Get()`s that duplicate and calls `runtime.SetFinalizer(clientStream, Close)` on an object that already has a live finalizer attached (from the other copy), which is a fatal runtime error and takes down the entire process.
This is a state-corruption crash: the second close doesn't just fail locally, it poisons the shared pool and crashes an **unrelated** request later.
### Reproduce
Any code path that closes the streamed response body twice, e.g.:
```go
resp := &protocol.Response{}
_ = cli.Do(ctx, req, resp) // client created with client.WithResponseBodyStream(true)
// SSE / chunked response
_ = resp.CloseBodyStream()
_ = resp.CloseBodyStream() // second close -> same object Put twice -> eventual fatal crash
```
A common real-world trigger: closing from both a `defer` and a `context`-cancellation goroutine that races to unblock a hung `Read`.
### Expected behavior
`clientRespStream.Close()` / `Response.CloseBodyStream()` should be safe to call multiple times (idempotent), or at least must never return the same object to the pool more than once. A double close should be a no-op, not a process-fatal error.
### Suggested fix
Add a `closed` guard so the body/pool logic runs at most once:
```go
type clientRespStream struct {
mu sync.Mutex
closed bool
r io.Reader
closeCallback func(shouldClose bool) error
}
func (c *clientRespStream) Close() (err error) {
c.mu.Lock()
defer c.mu.Unlock()
if c.closed {
return nil
}
c.closed = true
runtime.SetFinalizer(c, nil)
err = ext.ReleaseBodyStream(c.r)
if c.closeCallback != nil {
if err != nil {
hlog.SystemLogger().Warnf("error occurred during the stream body close: %s", err)
}
err = c.closeCallback(err != nil)
}
c.r = nil
c.closeCallback = nil
clientRespStreamPool.Put(c)
return
}
```
`convertClientRespStream()` must also reset `closed = false` when checking out from the pool. `ForceClose()` should be reconciled with the same flag.
### Notes
- The doc comment on `Close()` says *"MUST ensure it only be called when no longer use"*, so callers are technically expected to close exactly once. However, given that a caller mistake corrupts a shared pool and crashes a **different** request, an internal idempotency guard would make the library significantly more robust.
- The vulnerable code is identical on the current `develop` branch, so this is not fixed upstream.
### Environment
- Hertz version: `v0.10.3` (also present on `develop`)
- Go version: 1.x
- OS: Linux
Contributor guide
Research direction
Start in pkg/protocol/http1/resp/response.go, especially convertClientRespStream and clientRespStream.Close, then trace Response.CloseBodyStream and ForceClose. Reproduce the two-close path described in the issue and add coverage for repeated close and subsequent pooled-stream reuse. Done means duplicate closes do not corrupt the pool or cause a fatal finalizer error.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100