Skip to content

Conversation

@tusharkhandelwal8
Copy link
Contributor

This PR addresses the critical issue where Remote Config would fail to fetch new data and get stuck on default values after an iOS device was restored from a backup. It implements a two-part solution:

  1. Fixes a Logical Initialization Race to ensure the necessary resetUserDefaults logic runs.
  2. Fixes a Data Race on a global flag to eliminate a Thread Sanitizer warning.

@tusharkhandelwal8 tusharkhandelwal8 marked this pull request as ready for review October 25, 2025 08:11
@ncooke3
Copy link
Member

ncooke3 commented Oct 29, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request effectively resolves two race conditions in the Remote Config initialization process. The first fix correctly addresses a logical race condition by ensuring database setup completes before its state is read, using a blocking wait on the database operation queue. The second fix eliminates a data race on the gIsNewDatabase global flag by synchronizing all read and write access through a dedicated serial dispatch queue. The changes are well-implemented and include clear comments explaining the reasoning. I have one minor suggestion for code conciseness.

Copy link
Member

Choose a reason for hiding this comment

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

@tusharkhandelwal8 there are several dispatch_syncs going on here. If we're on the main queue when dispatching synchronously, we could hold up the main queue and risk deadlocks. Is there a way to make the synchronization of the property asynchronous?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is fine as-is. The work done on the synchronizing queue is only ever:

  • read a global variable
  • write a global variable
  • nothing (empty block to synchronize)

In the future a more ideal architecture would either be to initialize asynchronously and then overwrite the database with the backup-restored data, or just initialize synchronously on the main thread instead of doing async initialization and then using a locking mechanism to wait for it. @tusharkhandelwal8 if you have the time you could try profiling app launch with this patch to see how the dispatch_sync affects launch times.

Regardless, I'm fine with approving this as a temporary fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion on profiling!
I profiled the app launch with the dispatch_sync patch to assess the performance overhead. The synchronization block almost increased 0.3% of the total measured App Launch Time in the test environment. Given the extremely low impact, this synchronous block seems like a good solution for now.

I agree that a fully synchronous or fully asynchronous initialization would be the long-term architectural goal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants