fix: support binding a provider to multiple domains - #528
Conversation
📝 WalkthroughWalkthroughThe PR deduplicates provider initialization and shutdown across default and named bindings. It strengthens provider identity comparison, closes initialization channels after errors, adds context-aware lifecycle test utilities, and expands coverage for multi-binding and shutdown errors. ChangesProvider lifecycle handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenFeatureAPI
participant eventExecutor
participant FeatureProvider
Client->>OpenFeatureAPI: Register provider
OpenFeatureAPI->>eventExecutor: Check provider tracking
alt provider already tracked
OpenFeatureAPI-->>Client: Return closed completion channel
else provider is new
OpenFeatureAPI->>FeatureProvider: Initialize
FeatureProvider-->>OpenFeatureAPI: Return initialization result
OpenFeatureAPI-->>Client: Close completion channel
end
Client->>OpenFeatureAPI: Shutdown
OpenFeatureAPI->>FeatureProvider: Shutdown once per provider reference
FeatureProvider-->>OpenFeatureAPI: Return shutdown result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #528 +/- ##
==========================================
+ Coverage 86.45% 86.73% +0.28%
==========================================
Files 22 22
Lines 2119 2149 +30
==========================================
+ Hits 1832 1864 +32
+ Misses 242 241 -1
+ Partials 45 44 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Allow the same provider instance to be bound as the default provider and/or to multiple domains. Reuse the existing initialized provider instead of initializing it again. Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openfeature/context_aware_test.go (1)
208-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConfigure an actual slow shutdown.
slowShutdownProvideruses the default 10 ms shutdown delay. The 100 ms sleep therefore does not test replacement during a slow shutdown.Proposed test fix
- slowShutdownProvider := newTestContextAwareProvider(t, 10*time.Millisecond) + slowShutdownProvider := newTestContextAwareProvider(t, 10*time.Millisecond, 5*time.Second)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openfeature/context_aware_test.go` around lines 208 - 230, Update the “shutdown timeout handling” test to configure slowShutdownProvider with a shutdown delay substantially longer than the 100 ms wait, while keeping fastProvider unchanged. Use the existing newTestContextAwareProvider setup and preserve the assertions that provider replacement succeeds while the previous provider shuts down asynchronously.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openfeature/openfeature_api.go`:
- Around line 226-232: Ensure reused providers wait for the original
initialization result instead of receiving an immediately closed success
channel: update the tracked-provider path in
openfeature/openfeature_api.go:226-232 and the equivalent named-provider path at
openfeature/openfeature_api.go:264-270 to store and return the existing pending
result, propagating initialization failures. Add a blocking or failing Init test
in openfeature/openfeature_test.go:258-282 that binds the same provider to a
second target before the first initialization completes.
In `@openfeature/reference.go`:
- Around line 24-41: Use one identity policy for non-comparable providers across
providerReference.equals, its coverage in openfeature/reference_test.go:85-100,
and the handling in openfeature/event_executor.go:385-393. Replace
reflect.DeepEqual-based state comparison with a stable identity mechanism, or
consistently treat non-comparable copyable providers as distinct, ensuring
registration tracking and shutdown do not conflate separate providers or
repeatedly reinitialize the same provider.
---
Outside diff comments:
In `@openfeature/context_aware_test.go`:
- Around line 208-230: Update the “shutdown timeout handling” test to configure
slowShutdownProvider with a shutdown delay substantially longer than the 100 ms
wait, while keeping fastProvider unchanged. Use the existing
newTestContextAwareProvider setup and preserve the assertions that provider
replacement succeeds while the previous provider shuts down asynchronously.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: edfca913-6a26-4e50-a752-331cfe237e5f
📒 Files selected for processing (7)
openfeature/context_aware_test.goopenfeature/event_executor.goopenfeature/openfeature_api.goopenfeature/openfeature_test.goopenfeature/reference.goopenfeature/reference_test.goopenfeature/util_test.go
| // If the provider is already tracked, it has already been initialized. | ||
| // Return an already-closed channel so callers can proceed without waiting. | ||
| if tracked { | ||
| errCh := make(chan error, 1) | ||
| close(errCh) | ||
| return errCh, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wait for the original initialization before reporting a reused provider as ready.
Registration adds the provider to event tracking before initNew completes. A second binding can therefore see tracked == true, receive a closed channel, and return nil while the original Init is still running or has failed.
openfeature/openfeature_api.go#L226-L232: Store and return the existing provider initialization result instead of a closed success channel.openfeature/openfeature_api.go#L264-L270: Apply the same pending-initialization behavior for named providers.openfeature/openfeature_test.go#L258-L282: Add a blocking or failingInitcase that binds the same provider to the second target before the first initialization completes.
📍 Affects 2 files
openfeature/openfeature_api.go#L226-L232(this comment)openfeature/openfeature_api.go#L264-L270openfeature/openfeature_test.go#L258-L282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openfeature/openfeature_api.go` around lines 226 - 232, Ensure reused
providers wait for the original initialization result instead of receiving an
immediately closed success channel: update the tracked-provider path in
openfeature/openfeature_api.go:226-232 and the equivalent named-provider path at
openfeature/openfeature_api.go:264-270 to store and return the existing pending
result, propagating initialization failures. Add a blocking or failing Init test
in openfeature/openfeature_test.go:258-282 that binds the same provider to a
second target before the first initialization completes.
| // equals reports whether pr and other refer to the same provider. | ||
| // | ||
| // For providers whose dynamic type is comparable, equality is determined using | ||
| // Go's == operator. For non-comparable provider types (such as those containing | ||
| // maps or slices), equality falls back to reflect.DeepEqual. | ||
| // | ||
| // The providers must have the same dynamic type to be considered equal. | ||
| func (pr providerReference) equals(other providerReference) bool { | ||
| if pr.kind == reflect.Pointer && other.kind == reflect.Pointer { | ||
| if pr.typeOf == nil { | ||
| return false | ||
| } | ||
| if pr.typeOf != reflect.TypeOf(other.featureProvider) { | ||
| return false | ||
| } | ||
| if pr.typeOf.Comparable() { | ||
| return pr.featureProvider == other.featureProvider | ||
| } | ||
| return reflect.DeepEqual(pr.featureProvider, other.featureProvider) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
According to Go's reflect.DeepEqual documentation, do two separately constructed struct values that contain maps with equal contents compare equal?
💡 Result:
Yes, according to Go's reflect.DeepEqual documentation and implementation, two separately constructed struct values containing maps with equal contents will compare as equal [1][2][3]. Go's reflect.DeepEqual function performs a deep comparison of the data structures [1][4]. For map types, the documentation and source code specify that two maps are considered deeply equal if [1][2]: 1. They are both nil or both non-nil [1][2]. 2. They have the same length [1][2]. 3. Either they are the same map object (sharing the same memory address) or their corresponding keys map to deeply equal values [2][5]. Because reflect.DeepEqual recursively traverses the fields of a struct, if two structs have identical types and their corresponding fields are deeply equal, the structs themselves are considered deeply equal [2]. Consequently, when two structs contain maps, the function will successfully compare the contents of those maps, verifying that they contain the same key-value pairs, rather than simply checking if they refer to the same map object in memory [2][3].
Citations:
- 1: https://pkg.go.dev/reflect
- 2: https://go.googlesource.com/go/+/06b0a655a1f46abab2e3c173259ad184b557dd89/src/reflect/deepequal.go
- 3: https://dev.to/lucassha/using-reflect-deepequal-to-compare-data-structures-in-go-3dg0
- 4: https://go.dev/pkg/reflect/?m=old
- 5: https://go.googlesource.com/go/+/fcee6b930a01407527e3a0386af2ea7ac4e66d44/src/reflect/deepequal.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '(^|/)openfeature/(reference|reference_test|event_executor|openfeature_api|openfeature_test)\.go$' || true
printf '\nreference.go:\n'
cat -n openfeature/reference.go
printf '\nnearby tests:\n'
cat -n openfeature/reference_test.go | sed -n '1,140p'
printf '\nevent_executor isTracked and subscription init shutdown locations:\n'
rg -n "isTracked|newProviderRef|activeSubscriptions|Shutdown|featureProvider" openfeature -g '*event_executor.go' -g '*reference.go' -g '*openfeature_api.go'
printf '\nevent_executor relevant sections:\n'
cat -n openfeature/event_executor.go | sed -n '340,420p'Repository: open-feature/go-sdk
Length of output: 13076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'openfeature_api relevant sections:\n'
cat -n openfeature/openfeature_api.go | sed -n '200,270p'
cat -n openfeature/openfeature_api.go | sed -n '290,330p'
cat -n openfeature/openfeature_api.go | sed -n '404,440p'
printf '\npickSet usages:\n'
rg -n "pickSet|activeSubscriptions|setProvider|SetProvider|Shutdown" openfeature -g '*.go'
printf '\nGo reflect.DeepEqual behavior probe:\n'
go version || true
cat > /tmp/deep_equal_probe.go <<'GO'
package main
import (
"fmt"
"reflect"
)
type provider struct {
data map[string]string
}
func main() {
p1 := provider{data: map[string]string{"k": "v"}}
p2 := provider{data: map[string]string{"k": "v"}}
fmt.Println("type comparable:", reflect.TypeOf(p1).Comparable())
fmt.Println("reflect.DeepEqual:", reflect.DeepEqual(p1, p2))
}
GO
cd /tmp && go run deep_equal_probe.goRepository: open-feature/go-sdk
Length of output: 37658
Use one instance-identity policy for non-comparable providers.
reflect.DeepEqual treats distinct providers with the same map contents as equal, which is state equality, not provider identity. This can skip shutdown for a different provider registered with the same content and makes isTracked always reinitialize non-comparable providers. Use a stable identity mechanism for non-comparable providers, or treat non-comparable copyable providers as distinct at all call sites (reference.go, reference_test.go, event_executor.go).
📍 Affects 3 files
openfeature/reference.go#L24-L41(this comment)openfeature/reference_test.go#L85-L100openfeature/event_executor.go#L385-L393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openfeature/reference.go` around lines 24 - 41, Use one identity policy for
non-comparable providers across providerReference.equals, its coverage in
openfeature/reference_test.go:85-100, and the handling in
openfeature/event_executor.go:385-393. Replace reflect.DeepEqual-based state
comparison with a stable identity mechanism, or consistently treat
non-comparable copyable providers as distinct, ensuring registration tracking
and shutdown do not conflate separate providers or repeatedly reinitialize the
same provider.
1 participant
This PR
and/or to multiple domains. Reuse the existing initialized provider
instead of initializing it again.
Related Issues
Fixes #527