open-feature / open-feature/go-sdk

[BUG] AddHandler runs the immediate callback synchronously under the executor mutex: panics escape and re-entrant registration deadlocks

Open
#551 0 comments 0 reactions 0 assignees View on GitHub
bug Needs Triage
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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.