We previously had a hack in Python/C++ Protobuf to account for the fact that LazyField did not properly remember a custom DescriptorPool or MessageFactory that was set in the ParseContext at parse time.
The code has since been fixed to properly handle the case where ParseContext contains a custom DescriptorPool/MessageFactory. Removing the hack removes the data race under free threading.
PiperOrigin-RevId: 967236272
This change modifies all remaining `Dealloc()` functions to use `EraseIfEqual` if they were not already. This prevents the same race that was fixed for descriptors in cl/874084218.
PiperOrigin-RevId: 952273589
This change introduces a FreeThreadingMutex to PyDescriptorPool to guard access to the descriptor_options and descriptor_features hash maps.
PiperOrigin-RevId: 908115178
This change introduces a `LazyUniquePtr` class to manage the lazy initialization of `composite_fields` and `child_submessages` within `CMessage`. When Python's GIL is disabled, `LazyUniquePtr` uses atomic operations to ensure thread-safe initialization of these maps, preventing data races when multiple threads concurrently access fields that trigger their creation. A new test case is added to reproduce and verify the fix for a race condition in `GetFieldValue`.
We are forced to use placement new and placement delete for the `LazyUniquePtr` members, because the `CMessage` struct is currently not properly constructed or destroyed. This makes the code a bit awkward, but changing the construction/destruction of CMessage seemed like too much to bite off in this CL.
PiperOrigin-RevId: 891703488
This CL concerns the following functions in C++:
```c++
class Reflection {
const Message& GetRepeatedMessage(const Message& message,
const FieldDescriptor* field,
int index) const;
Message* MutableRepeatedMessage(Message* message,
const FieldDescriptor* field,
int index) const;
}
```
Suppose a Python program contains the following code:
```
def foo(msg: MyMessage):
submsg = msg.repeated_foo[2] # (A)
if also_mutate:
submsg.abc = 1 # (B)
```
For line (A), we currently call `MutableRepeatedMessage()` in C++ to
obtain the submessage pointer, because the resulting `submsg` object in
Python is a conceptually mutable object that will require a non-const
pointer if/when the program hits line (B).
The `MutableRepeatedMessage(Message* msg, ...)` function in C++
currently mutates `msg` by setting the hasbit of the repeated field. But
this seems unnecessary, as the function requires that the requested
sub-message already exists; it does not create a new message. So it
should not be necessary to touch the hasbit of the message, and a TGP
confirms that all tests pass if we remove this call.
Once the `SetHasBitForRepeated()` call is removed, the `MutableRepeatedMessage()`
function no longer actually mutates the `msg` argument. This makes it
effectively safe to call concurrently (ie. it will no longer trigger
Undefined Behavior in C++, and the TSAN errors will go away), which
is why it fixes the free threading test. But it leaves us in an odd
state where we are still passing a non-`const` pointer to the same
message to two functions concurrently, which is not allowed under a
normal thread-compatible contract.
An alternate solution would be to call `GetRepeatedMessage()` instead,
and `const_cast<>` away the const in the returned `const Message&`.
But this is also violates the contract: users should not be casting
away `const`.
The root cause of this odd situation is that
`MutableRepeatedMessage(Message* msg)` requires a non-`const` `msg` not
because it actually *mutates* `msg`, but because it is trying to
propagate the const-ness of `msg` to all of its children. The proto API
generally guarantees that a const message prevents mutation of not just
the top-level message, but of the entire tree of messages. If someone
passes you a `const` message pointer, that is supposed to render the
entire tree of messages under it immutable through that pointer.
What we wish we could express in `MutableRepeatedMessage(Message* msg)`
is: "this function requires that `msg` is mutable, but this function
will not actually mutate it, and is therefore safe to call concurrently."
But there is no way of expressing this in C++.
PiperOrigin-RevId: 886830933
Prior to this CL, it was possible for the following sequence to occur:
|Thread 1|Thread 2|
|--------|--------|
|`obj = NewDescriptor(desc)`||
|`InsertCache(desc, obj)`||
|`Py_DECREF(obj)` (to 0)||
|`Dealloc(obj) {`||
||`LookupCache(desc) -> obj`|
||`Py_INCREF(obj)`|
|` DeleteFromCache(obj)`||
|`}`||
||`Py_DECREF(obj)`|
||`Dealloc(obj)`|
This could lead to double-`Dealloc()` calls on a single object. These calls could race, leading to TSAN failures.
We should look deeper into whether `GcTraverse()` and `GcClear()` still need critical sections.
PiperOrigin-RevId: 874084218
