[Auth] Fix a race condition - #15951

Code Review

This pull request effectively addresses a race condition on the User.providerData property by introducing a serial DispatchQueue to synchronize all read and write access to the underlying providerDataRaw dictionary. The changes are applied consistently across all access points in User.swift and UserProfileUpdate.swift. The addition of UserThreadSafetyTests.swift with a concurrent read/write test is a great way to validate the fix. I have one minor suggestion to make the new test even more robust.

Code Review

This pull request introduces a serial dispatch queue to address a race condition on the User.providerData getter, which is a solid approach to improving thread safety. The protection has been applied to the getter and several methods that modify user properties. While this is a good step, the application of this protection seems incomplete, as some property accesses remain unsynchronized, potentially leading to other race conditions. I've left a couple of comments highlighting these areas. The addition of a new thread safety test is excellent for verifying the fix.

Code Review

This pull request effectively addresses a race condition by introducing a serial dispatch queue to protect access to User properties. The synchronization has been applied to various read and write paths, and a new test suite validates the fix under concurrent access. However, the implementation has a minor flaw: the encode(with:) method's synchronization is incomplete, as some properties are still accessed outside the protected block, leaving a potential for race conditions during serialization. I've left a specific comment on this.

Code Review

This pull request addresses a race condition in the User class by introducing a serial dispatch queue to synchronize access to its properties. The changes correctly wrap write operations and some read operations in synchronized blocks, which is a solid step towards making the class thread-safe. The addition of UserThreadSafetyTests is also excellent for verifying the fix under concurrent load.

My review includes two main points. The most critical one is that while write access to many properties is now synchronized, read access for most of them remains unsynchronized, which can still lead to data races. I've provided a detailed comment on how to make the class fully thread-safe. I've also included a minor suggestion for a small performance optimization by combining synchronized blocks.

Overall, this is a good fix, but completing the thread-safety implementation is important for correctness.

Code Review

This pull request effectively addresses a race condition in the User class by introducing a serial dispatch queue to synchronize access to its properties. The changes are well-implemented across property accessors, mutating functions, and serialization. The addition of UserThreadSafetyTests is a great way to validate the fix. I have one suggestion to ensure complete thread safety for all property modifications.

Code Review

This pull request does a good job of addressing a race condition in the User class by introducing a serial dispatch queue to protect property access. The changes are applied consistently across property getters, setters, and serialization methods, and the new thread safety tests are a valuable addition. However, I've identified a critical issue where a nested synchronous dispatch to the same serial queue will cause a deadlock. Please address this to ensure the stability of the fix.

Code Review

This pull request effectively addresses a race condition in the User class by introducing a serial dispatch queue to synchronize access to its properties. The changes are comprehensive, covering property getters, mutation points, and serialization logic. The introduction of UserThreadSafetyTests is a great addition to validate the fix.

I have one suggestion regarding the access level of the new backing properties to further improve encapsulation and guarantee thread safety.

Code Review

This pull request effectively addresses a race condition in the FirebaseAuth module by introducing a private serial dispatch queue (propertyAccessQueue) to synchronize access to critical User properties. The implementation correctly converts properties to computed properties with synchronized getters and setters, and updates all relevant methods to use this queue for thread-safe access. The addition of UserThreadSafetyTests.swift provides excellent validation for the fix under concurrent read/write conditions, demonstrating a thorough approach to ensuring thread safety.

2 things I'm thinking about:

• forward/backwards compatibility of data encodings
• priority inversions/main queue blocking from the sync dispatches

LGTM with a few suggestions.

LGTM. The coder logic looks safe, and I'm not seeing any nested syncs. I suppose the main thread will block depending on where these User APIs are called from, but such pauses should be very quick.