Skip to content

[FSSDK-11776] fixing memory leak with project #372

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alexjoeyyong
Copy link

@alexjoeyyong alexjoeyyong commented Aug 22, 2025

Summary

This pull request introduces a robust caching mechanism to the Optimizely::Project class to prevent memory leaks caused by repeated SDK initialization, especially in high-throughput environments (like Rails apps). It adds a static instance cache, ensures proper cleanup of background threads, and provides new APIs for cache management. Extensive tests and documentation are included to guarantee safe usage and clarify best practices.

Key changes include:

Memory Leak Prevention & Caching Logic

  • Introduced a class-level instance cache in Optimizely::Project, with thread-safe access, to ensure that repeated initialization with the same static datafile returns the same instance, preventing unbounded thread growth and memory leaks. Instances are only cached for static datafile configurations and not for dynamic options (e.g., sdk_key, custom managers, etc.).
  • Added .get_or_create_instance, .clear_instance_cache!, and .cached_instance_count APIs to manage cached instances, and ensured that closing an instance removes it from the cache [1] [2].

Resource Cleanup & Thread Management

  • Implemented a finalizer using ObjectSpace.define_finalizer to ensure background threads (config_manager, event_processor, odp_manager) are cleaned up even if close is not called explicitly, further reducing risk of leaks.
  • Updated the close method to remove the instance from the cache and stop all background resources safely.

Testing & Safety Guarantees

  • Added comprehensive specs in spec/project_caching_spec.rb and spec/memory_leak_prevention_spec.rb to verify caching logic, proper resource cleanup, thread stability, and to ensure that only safe configurations are cached [1] [2].

Documentation

  • Added a new .github/copilot-instructions.md file detailing the SDK architecture, memory safety best practices, and correct usage patterns to prevent memory leaks in production environments.

These improvements make the SDK safer for production use, especially in multi-threaded and long-running server environments.


Memory leak prevention and caching:

  • Added class-level instance cache and .get_or_create_instance API to Optimizely::Project for safe reuse of static datafile configurations, preventing memory leaks from repeated initialization.
  • Ensured only safe configurations are cached (static datafile, no dynamic options), and provided APIs for clearing and monitoring the cache.

Resource cleanup and thread management:

  • Added a finalizer to clean up background threads if close is not called, and updated close to remove the instance from the cache [1] [2].

Testing and safety guarantees:

  • Introduced new specs to verify caching, thread stability, correct cleanup, and memory safety in various usage patterns (spec/project_caching_spec.rb, spec/memory_leak_prevention_spec.rb) [1] [2].

Documentation and usage guidance:

  • Added .github/copilot-instructions.md with architecture overview, memory leak prevention advice, and best practices for safe SDK usage.

Test plan

Pass automated tests

Issues

@alexjoeyyong alexjoeyyong changed the title BUG-8180 fixing memory leak with project [BUG-8180] fixing memory leak with project Aug 22, 2025
@alexjoeyyong alexjoeyyong changed the title [BUG-8180] fixing memory leak with project [FSSDK-11776] fixing memory leak with project Aug 22, 2025
@alexjoeyyong alexjoeyyong marked this pull request as ready for review August 22, 2025 14:41
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a comprehensive caching mechanism to the Optimizely::Project class to prevent memory leaks caused by repeated SDK initialization in high-throughput environments like Rails applications. The implementation includes thread-safe instance caching, automatic resource cleanup, and extensive testing to ensure production safety.

Key changes include:

  • Added class-level instance caching with .get_or_create_instance API to prevent memory leaks from repeated initialization
  • Implemented automatic resource cleanup using finalizers and enhanced the close method to remove instances from cache
  • Added comprehensive test coverage for caching behavior, memory leak prevention, and thread management

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
lib/optimizely.rb Implements the core caching mechanism with thread-safe cache management, cache key generation, and automatic cleanup
spec/project_caching_spec.rb Tests the caching API behavior, instance reuse, and cache invalidation scenarios
spec/memory_leak_prevention_spec.rb Tests thread creation prevention, resource cleanup, and production usage patterns
.github/copilot-instructions.md Provides comprehensive documentation on SDK architecture and memory leak prevention best practices
Comments suppressed due to low confidence (1)

spec/memory_leak_prevention_spec.rb:115

  • [nitpick] The magic number 10 for thread count variance is not explained. Consider adding a comment explaining why this specific threshold was chosen or making it a named constant.
      instance1 = Optimizely::Project.get_or_create_instance(datafile: datafile)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

instance_cache.each_value do |instance|
next if instance.stopped

instance.instance_variable_set(:@stopped, true)
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using instance_variable_set to modify internal state bypasses encapsulation and could lead to inconsistent state. Consider adding a private method to handle this state change properly.

Suggested change
instance.instance_variable_set(:@stopped, true)
instance.send(:mark_stopped!)

Copilot uses AI. Check for mistakes.

config_manager.stop! if config_manager.respond_to?(:stop!)
event_processor.stop! if event_processor.respond_to?(:stop!)
odp_manager.stop! if odp_manager.respond_to?(:stop!)
rescue
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bare rescue clause catches all exceptions including SystemExit and Interrupt. Consider rescuing StandardError instead to avoid catching system-level exceptions.

Suggested change
rescue
rescue StandardError

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant