Skip to content

Commit ebe9513

Browse files
fix: Synchronize provider state access on the state lock (#54)
`getState()` synchronized on the `state` field's value instead of the `stateLock` monitor, so reads were never mutually exclusive with writes. - `getState()` now locks `stateLock`, matching `setState` - `initialize` sets the ready state through `setState` and reads it through `getState` instead of touching the field unlocked - No behavior change for callers beyond correct visibility of state transitions @cursor review <details> <summary>Implementation details</summary> **Root cause** `synchronized (state)` locks on the `ProviderState` enum constant currently referenced by the field. A reader holding the monitor of `ProviderState.NOT_READY` does not exclude a writer holding `stateLock`, so state transitions had no happens-before relationship with reads. Because enum constants are JVM-wide singletons, it also meant contending on a monitor shared with any other code that happens to lock the same constant. `initialize` also wrote `state = ProviderState.READY` and later read `state` without any lock, on a different thread than the data source status listener that mutates it. **Alternatives considered** Making the field `volatile` would fix visibility, but the `VALID` branch of `handleDataSourceStatus` needs a compare-and-set over the field, so the lock is still required; keeping a single lock is simpler than mixing both. **Testing** `./gradlew test` — all 44 tests pass. The existing `LifeCycleTest` cases already assert the state transitions this lock protects (`NOT_READY` → `READY`, and → `ERROR` on a failed data source); a test cannot deterministically observe the previous incorrect locking, so no new test is added. No visual preview applies — this is a server-side provider change. </details> Link to Devin session: https://app.devin.ai/sessions/0c452d209ec54b068ba120b4c92b8f6c Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Fixes incorrect locking around the OpenFeature provider lifecycle state so reads and writes use the same monitor. > > `getState()` now synchronizes on `stateLock` instead of on the `ProviderState` enum value held in the field, which did not exclude concurrent updates from `setState` and could contend on JVM-wide enum monitors. **`initialize`** routes the early-ready path through `setState(READY)` and the readiness check through `getState()` instead of assigning or reading the `state` field without the lock. > > Callers should see the same transitions, with correct visibility and mutual exclusion when the data source listener updates state on another thread. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit dfc2c89. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 parent 1f71ef3 commit ebe9513

1 file changed

Lines changed: 3 additions & 3 deletions

File tree

  • src/main/java/com/launchdarkly/openfeature/serverprovider

‎src/main/java/com/launchdarkly/openfeature/serverprovider/Provider.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultVa
139139
@Deprecated
140140
@Override
141141
public ProviderState getState() {
142-
synchronized (state) {
142+
synchronized (stateLock) {
143143
return state;
144144
}
145145
}
@@ -149,7 +149,7 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
149149
// If we are ready, then set the state. Don't return, because we still need to listen for future
150150
// changes.
151151
if (client.isInitialized()) {
152-
state = ProviderState.READY;
152+
setState(ProviderState.READY);
153153
}
154154

155155
var completer = new CompletableFuture<Boolean>();
@@ -163,7 +163,7 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
163163
handleDataSourceStatus(res, completer);
164164
});
165165

166-
if(state == ProviderState.READY) {
166+
if (getState() == ProviderState.READY) {
167167
return;
168168
}
169169

0 commit comments

Comments
 (0)