-
Notifications
You must be signed in to change notification settings - Fork 489
feat(openfeature/provider): Add ability to pass context into openfeature provider to support cancellation #4087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add ability to pass context into openfeature provider to support cancellation - Update to OpenFeature v1.17.0 from pinned pre-release version - Implement ContextAwareStateHandler with InitWithContext and ShutdownWithContext - Add comprehensive timeout tests for SetProviderWithContextAndWait and ShutdownWithContext - Simplify InitWithContext implementation using channels instead of sync.Cond - Replace complex mutex lock/unlock cycles with clean channel-based signaling - All tests passing with improved error handling for test scenarios
88d9117 to
ef3e3de
Compare
- Reduce initialization timeout from 30s to 5s for faster timeouts - Reduce shutdown timeout from 10s to 5s for consistency - Revert InitWithContext from channel approach back to sync.Cond - sync.Cond supports multiple broadcasts for ongoing config updates - All timeout tests passing with improved responsiveness
openfeature/provider.go
Outdated
| // Check if context was cancelled | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| default: | ||
| } | ||
|
|
||
| // Use a condition variable with context support | ||
| // We need to handle the case where context gets cancelled while waiting | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| defer close(done) | ||
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
| p.configChange.Wait() | ||
| }() | ||
|
|
||
| // Temporarily unlock to allow the configuration update and context handling | ||
| p.mu.Unlock() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| // Relock before returning | ||
| p.mu.Lock() | ||
| return ctx.Err() | ||
| case <-done: | ||
| // Configuration might have been updated, relock and loop to check | ||
| p.mu.Lock() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we made the inside of the loop into it's own function so we can defer p.mu.Lock() instead of locking on each branch of the select statement. Do you agree ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea thanks for mentioning it.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Tests failed on this commit 39cbc7b:
What to do next?
|
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Motivation
context.Contextand cancel or timeout if the service is down. The customer impact is that the server can hang which is an extremely negative customer experience.1.17.0What does this PR do?
1.17.0Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!