Uh oh!
There was an error while loading. Please reload this page.
fix(proto-plus): make Marshal thread-safe and handle race conditions - #17774
Conversation
- Use Double-Checked Locking and Copy-on-Write in Marshal.__new__ - Use getattr safely in BaseMarshal.get_rule - Add tests for concurrency scenarios Fixes#15100
There was a problem hiding this comment.
Code Review
This pull request introduces thread-safety improvements to the Marshal class, implementing a threading lock, double-checked locking, and copy-on-write dictionary updates to prevent race conditions and dictionary mutation errors during iteration. It also adds safety checks for uninitialized instances and a comprehensive test suite. The review feedback recommends using unittest.mock.patch.dict in the tests to safely modify Marshal._instances and avoid potential test pollution if a test fails.
Uh oh!
There was an error while loading. Please reload this page.
…tests Follows reviewer suggestion to avoid potential test pollution.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Thread-safe Marshal Initialization
Problem
A RuntimeError saying
dictionary changed size during iterationcan occur randomly inBaseMarshal.get_rule. This happens because one thread is reading the_instancesdictionary while another thread is adding a new instance to it. This is common when using features like Firestore's on_snapshot in a background thread.Solution
Marshal.__new__. It uses a technique called "double-checked locking" to make sure only one thread creates a new instance at a time without slowing down normal reads.BaseMarshal.get_ruleto avoid trying to read rules from an instance that has been registered but has not finished initializing yet.Notes to Reviewers
test_marshal_thread_safety.pyhas been added to cover these concurrency scenarios.Fixes#15100