open-feature / open-feature/go-sdk
[BUG] AddHandler runs the immediate callback synchronously under the executor mutex: panics escape and re-entrant registration deadlocks
- Dominant language
- Go
- Stars
- 250
- Forks
- 62
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 25
Description
## Observed behavior
`AddHandler` and `AddClientHandler` hold `e.mu` and then call `emitOnRegistration`:
https://github.com/open-feature/go-sdk/blob/6b0824c6c7b9b69e40f26aedcc4406ad4d3a5cec/openfeature/event_executor.go#L71-L83
…which invokes the callback **inline, while the mutex is still held**:
https://github.com/open-feature/go-sdk/blob/6b0824c6c7b9b69e40f26aedcc4406ad4d3a5cec/openfeature/event_executor.go#L154-L179
Every other dispatch goes through `executeHandler`, which runs the callback on its own goroutine wrapped in `recover()`:
https://github.com/open-feature/go-sdk/blob/6b0824c6c7b9b69e40f26aedcc4406ad4d3a5cec/openfeature/event_executor.go#L365-L383
Two consequences, both reproduced:
```
PANIC propagated out of AddHandler: handler boom
DEADLOCK: AddHandler from within an immediately-run handler blocked >2s
```
1. A handler that panics crashes the caller of `AddHandler`. The *same handler* invoked via the normal event path is protected by `executeHandler`'s `recover()` — so whether a panic is survivable depends on registration timing.
2. A handler that registers another handler (or calls `client.AddHandler`) deadlocks on the non-reentrant `e.mu`. Registering a readiness handler that wires up further handlers is a reasonable thing to do.
## Expected Behavior
Per [5.2.5](https://openfeature.dev/specification/sections/events#requirement-525): *"If a `handler function` terminates abnormally, other `handler functions` **MUST** run."* Routing `emitOnRegistration` through `executeHandler` (or otherwise releasing the lock and recovering) addresses both.
Every sibling SDK isolates the immediate-run handler:
- **Java** — `eventSupport.runHandler(...)` submits to a `taskExecutor` with try/catch, the same path as normal dispatch. (Java also holds a write lock during registration, but because the handler is dispatched to an executor, neither panic-propagation nor deadlock occurs.)
- **JS** — `try { handler(...) } catch (err) { this._logger?.error(...) }`.
- **Python** — `_submit_handler` → thread pool with a blanket `except Exception`.
## Steps to reproduce
Panic escaping:
```go
api.SetProviderAndWait(ctx, readyProvider)
cb := func(d of.EventDetails) { panic("handler boom") }
api.AddHandler(of.ProviderReady, &cb) // panic escapes; no recover
```
Deadlock:
```go
api.SetProviderAndWait(ctx, readyProvider)
cb := func(d of.EventDetails) {
inner := func(of.EventDetails) {}
api.AddHandler(of.ProviderStale, &inner) // blocks forever on e.mu
}
api.AddHandler(of.ProviderReady, &cb)
```
Contributor guide
Research direction
Start in openfeature/event_executor.go at AddHandler, AddClientHandler, emitOnRegistration, and executeHandler. Trace the registration path and verify the panic and re-entrant registration reproductions no longer escape or deadlock, with regression coverage for both behaviors.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100