Skip to content

Commit 1a0d39e

Browse files
leoromanovskytoddbaertsahidvelji
authored
feat: Adds ability to pass a context.Context to provider with new public methods. (#442)
* Adds ability to pass a context.Context to provider with new public methods. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * improve: enhance error context handling for provider initialization - Distinguish between context cancellation/timeout and provider-specific errors - Provide clearer error messages for different failure scenarios - Maintain consistency between context-aware and regular handlers Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * improve: add context validation before provider initialization - Check if context is already cancelled/expired before starting initialization - Provide clear error messages for pre-cancelled contexts - Prevent unnecessary initialization attempts on expired contexts Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * improve: enhance error wrapping with better error chains - Add context to provider setup errors including provider name and domain - Use fmt.Errorf with %w verb to maintain error chains for better debugging - Provide more informative error messages for troubleshooting Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * test: make timeout values more realistic for CI environments - Increase provider initialization delays to 50-800ms for better test stability - Extend context timeouts to 100ms-1s to accommodate CI timing variations - Maintain proper timeout ratios to ensure tests remain deterministic - Improve async test timing tolerance to reduce flaky test failures Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * docs: add comprehensive documentation with examples and best practices - Add detailed interface documentation for ContextAwareStateHandler with implementation example - Include best practices for timeout values and context handling - Provide practical usage examples for all context-aware functions - Document use cases for async vs sync initialization patterns - Add multi-tenant/microservice architecture examples for named providers Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * fix: update tests to work with improved error wrapping - Use errors.Is() instead of direct equality for context error checking - Add errors import to support proper error unwrapping in tests - Maintain test reliability while preserving enhanced error context Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * fix: add missing SetNamedProviderWithContextAndWait interface method - Add SetNamedProviderWithContextAndWait to evaluationImpl interface - Implement SetNamedProviderWithContextAndWait method in evaluationAPI - Update public function to call correct implementation method - Add corresponding mock implementation for tests - Resolves interface completeness issue noted in PR review Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * refactor: eliminate code duplication in provider initialization functions - Create unified initNewAndShutdownOldInternal function with optional context - Make initNewAndShutdownOld and initNewAndShutdownOldWithContext thin wrappers - Reduces ~35 lines of duplicated logic while maintaining identical behavior - Uses context pointer to differentiate between context-aware and regular initialization - Addresses maintainer feedback about code duplication Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * docs: convert code examples to testable Example functions - Create example_context_test.go with testable Example functions for all context methods - Examples demonstrate real usage patterns: async setup, sync setup, multi-tenant, critical services - Simplify doc comments to focus on behavior descriptions rather than code blocks - Examples are automatically tested and serve as living documentation - Addresses maintainer feedback about preferring Example functions over doc comments Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * feat: add ShutdownWithContext for complete context-aware provider lifecycle - Add ShutdownWithContext method to ContextAwareStateHandler interface - Update shutdown logic to use context-aware shutdown with 10s timeout when available - Fall back to regular Shutdown for backward compatibility - Add comprehensive tests for context-aware shutdown behavior - Update test provider to implement ShutdownWithContext - Add example demonstrating context-aware provider lifecycle - Completes solution to issue #389 by supporting context in both Init and Shutdown Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * fixup: formatting Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * feat: add global ShutdownWithContext for application termination Added ShutdownWithContext function to IEvaluation interface and global openfeature package for graceful application shutdown with timeout control. This addresses maintainer feedback about providing context-aware shutdown for application termination scenarios. Changes: - Add ShutdownWithContext method to IEvaluation interface - Implement context-aware shutdown in evaluationAPI - Add global ShutdownWithContext function in openfeature.go - Add comprehensive tests for global shutdown scenarios - Add example demonstrating global shutdown usage - Update mock interfaces for new functionality The implementation properly handles both context-aware and regular providers, ensuring backward compatibility while enabling timeout control during application shutdown. Signed-off-by: leo.romanovsky <leo.romanovsky@datadoghq.com> Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * refactor: improve context handling based on Sahid's feedback Address Sahid's code review feedback to follow Go best practices: - Change initNewAndShutdownOldInternal to use context.Context instead of *context.Context - Replace nil context usage with context.Background() - Simplify conditional logic by always using initializerWithContext - Remove redundant initializer function that was just a wrapper Changes: - Fix function signature to use value type instead of pointer - Update all callers to pass contexts directly - Remove unnecessary nil checks and conditional branching - Clean up unused helper function Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * fix: resolve context closure bug in async goroutine The async goroutine was capturing ctx and newProvider by reference in the closure, creating a race condition where the goroutine could see modified values (like cancelled contexts) instead of the values that existed when the goroutine was created. Fixed by passing ctx, newProvider, and clientName as explicit goroutine parameters instead of capturing them by reference. This ensures the goroutine operates on the values that existed when the async operation was initiated, preventing timing-dependent failures. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * refactor: eliminate unnecessary function wrapper Remove initNewAndShutdownOldWithContext function which was just a pass-through to initNewAndShutdownOldInternal. Update callers to use initNewAndShutdownOldInternal directly. This reduces code complexity and eliminates an unnecessary layer of indirection. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * refactor: clarify function naming for better maintainability Rename functions to reflect their actual roles: - initNewAndShutdownOldInternal → initNewAndShutdownOld (main implementation) - initNewAndShutdownOld → initNewAndShutdownOldLegacy (background context wrapper) This makes it clear which function is the primary implementation and which is the legacy compatibility wrapper. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * cleanup: remove bug demonstration test Remove context_closure_bug_test.go since the context closure bug has been fixed. The test was created to demonstrate the issue and is no longer needed. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * refactor: eliminate magic number with documented constant Replace hardcoded 10-second timeout with DefaultShutdownTimeout constant. This makes the timeout value explicit and documents the reasoning behind the choice of 10 seconds for provider shutdown operations. The constant prevents indefinite hangs during application termination while allowing sufficient time for network cleanup operations. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * Update example_context_test.go Co-authored-by: Sahid Velji <sahidvelji@gmail.com> Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * fix: make shutdown timeout private and fix example package references - Change DefaultShutdownTimeout to private defaultShutdownTimeout constant - Add timeout documentation to the initNewAndShutdownOld function - Fix example_context_test.go to properly use openfeature package prefix - Add missing import for openfeature package in examples - Update all function calls to use openfeature.FunctionName() syntax This follows Go best practices by keeping implementation details private and properly demonstrates the public API in examples. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * improve: use errors.Join for shutdown error collection Replace first-error-only pattern with errors.Join to collect and return all shutdown errors. This provides users with complete information about all providers that failed to shut down cleanly, rather than stopping at the first failure. This is especially useful during application shutdown when you want visibility into all the problems that occurred across multiple providers. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * erka feedback; migrate existing methods to common helpers Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * tangenti feedback; fix context handling and add comprehensive tests - Fix shutdown context propagation to use passed context with smart timeout extension - Remove unnecessary pre-initialization context check as InitWithContext handles it - Simplify error handling logic to avoid redundant context checks - Add 330+ lines of comprehensive tests covering context propagation, error handling, and edge cases - Validate all fixes with new test providers and scenarios Addresses all technical concerns raised in review feedback. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * sahidvelji feedback; remove obsolete tests and cleanup dead code - Remove obsolete test for timeout extension behavior that no longer exists - Remove unused defaultShutdownTimeout constant and time import - Update comment that referenced removed defaultShutdownTimeout - Clean up 34 lines of dead code while maintaining functionality Completes simplification of shutdown context logic per reviewer feedback. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * fix lint Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> --------- Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: leo.romanovsky <leo.romanovsky@datadoghq.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: Sahid Velji <sahidvelji@gmail.com>
1 parent c2034e5 commit 1a0d39e

File tree

7 files changed

+1321
-54
lines changed

7 files changed

+1321
-54
lines changed

0 commit comments

Comments
 (0)