Skip to content

fix: improve thread-safe and atomic writing#204

Open
vazarkevych wants to merge 6 commits into
growthbook:mainfrom
vazarkevych:fix/improve-thread-safety
Open

fix: improve thread-safe and atomic writing#204
vazarkevych wants to merge 6 commits into
growthbook:mainfrom
vazarkevych:fix/improve-thread-safety

Conversation

@vazarkevych

Copy link
Copy Markdown
Collaborator

Summary

This PR addresses thread-safety issues across several core components of the GrowthBook Java SDK.

Changes

GBFeaturesRepository

  • Replaced mutable fields with AtomicBoolean, AtomicLong, AtomicReference where appropriate
  • Changed ArrayList for refresh callbacks to CopyOnWriteArrayList
  • Marked refreshStrategy and sseRequest as volatile
  • Replaced sseHttpClient and sseEventSource plain fields with AtomicReference

FileCachingManagerImpl

  • Replaced direct file writes with write-to-temp + atomic Files.move(ATOMIC_MOVE) pattern to prevent partial reads during concurrent access
  • Wrapped both saveContent and loadCache with class-level synchronization

GrowthBook

  • Replaced HashMap with ConcurrentHashMap for assigned experiments map
  • Replaced ArrayList with CopyOnWriteArrayList for callbacks
  • Marked callbacks and evaluationContext as volatile

GrowthBookClient

  • Marked static repository field as volatile
  • Wrapped initialize() with synchronized block to prevent double-initialization
  • Replaced HashMap/ArrayList with ConcurrentHashMap/CopyOnWriteArrayList
  • Marked globalContext as volatile

GrowthBookUtils

  • Extracted fireSubscriptions from both GrowthBook and GrowthBookClient into a shared utility method to eliminate duplication

InMemoryStickyBucketServiceImpl

  • Minor synchronization improvements

NativeJavaGbFeatureRepository

  • Fixed GbCacheManager initialization order

@madhuchavva madhuchavva left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Public getter compatibility issue found.

//@Getter
@Getter
private Map<String, Feature<?>> parsedFeatures = new HashMap<>();
private final AtomicReference<Map<String, Feature<?>>> parsedFeatures = new AtomicReference<>(new HashMap<>());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Preserve the existing public getter return types

These atomics are exposed through Lombok-generated getters, so existing public APIs change shape. For example, getParsedFeatures() becomes AtomicReference<Map<String, Feature<?>>> instead of Map<String, Feature<?>>, getParsedSavedGroups() becomes AtomicReference<JsonObject>, and getInitialized() becomes AtomicBoolean. That is a source/binary compatibility break for a thread-safety fix. Please keep the atomic fields private and add explicit getters that preserve the existing return types.

@madhuchavva madhuchavva left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes for the public API compatibility issue noted inline.

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.

2 participants