cloudwego / cloudwego/hertz

`fatal error: runtime.SetFinalizer: finalizer already set` — `clientRespStream.Close()` is not idempotent and double-Put's into the pool

Open
#1,547 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.