Ruby: Only add RepeatedField instance to cache once fully initialized#13054
Ruby: Only add RepeatedField instance to cache once fully initialized#13054casperisfine wants to merge 1 commit intoprotocolbuffers:mainfrom
Conversation
Fix: protocolbuffers#11968 Since calling a method on WeakMap allow the RubyVM to switch threads, adding a non-fully initialized object into the cache allow another thread from using it. Note: with this fix, the other two thread may create their own wrapper for the same underlying Array. We don't know if it's a big deal or not. Co-Authored-By: Peter Zhu <[email protected]>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Ah damn it. Crediting @peterzhu2118 as co-author is making the CLA bot nervous -_-. Peter could you sign the CLA? |
|
I signed the CLA. |
|
Thanks so much for debugging and fixing this. This pattern also occurs in the following place, which will also need the same change: protobuf/ruby/ext/google/protobuf_c/map.c Lines 96 to 105 in 2cf94fa I wish there was a way to atomically perform a "get or insert" operation on the WeakMap, so we could avoid the duplicate wrapper situation. Duplicate wrappers are mostly benign, but they are observable by the user, eg. with Would it be possible to add a "get or insert" operation to |
haberman
added
the
🅰️ safe for tests
|
After this fix in debug builds we may start to fail the assert here: I think the best solution is to have |
Ok, that won't quite work since a racing thread can call |
You can implement that yourself in Ruby with a mutex. If I read the code correctly the "secondary map" codepath uses a mutex, so similar, but it's easier to do in ruby (untested pseudo-code): class MyWeakMap
def initialize
@map = ObjectSpace::WeakMap.new
@mutex = Mutex.new
end
def getset(key, new_value)
return old_value if old_value = @map[key]
@mutex.synchronize do
@map[key] ||= new_value
end
end
end |
Reading the code again, I think this case was already possible before since we were calling Even with All this to say, I think this is a different concern, and shouldn't prevent the fix from being merged. |
Agreed, but the asserts are a really issue that we should remove |
Superseeds: protocolbuffers#13054 The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory. To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations. Instead we can replace `ObjectCache_Add` by a `getset` type of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance. Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The `ObjectCache` instance is also exposed as `Google::Protobuf::OBJECT_CACHE` to better allow to debug potential memory issues.
Superseeds: protocolbuffers#13054 The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory. To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations. Instead we can replace `ObjectCache_Add` by a `getset` type of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance. Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The `ObjectCache` instance is also exposed as `Google::Protobuf::OBJECT_CACHE` to better allow to debug potential memory issues.
Superseeds: protocolbuffers#13054 The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory. To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations. Instead we can replace `ObjectCache_Add` by a `getset` type of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance. Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The `ObjectCache` instance is also exposed as `Google::Protobuf::OBJECT_CACHE` to better allow to debug potential memory issues.
Superseeds: protocolbuffers#13054 The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory. To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations. Instead we can replace `ObjectCache_Add` by a `getset` type of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance. Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The `ObjectCache` instance is also exposed as `Google::Protobuf::OBJECT_CACHE` to better allow to debug potential memory issues.
Superseeds: protocolbuffers#13054 The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory. To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations. Instead we can replace `ObjectCache_Add` by a `getset` type of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance. Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The `ObjectCache` instance is also exposed as `Google::Protobuf::OBJECT_CACHE` to better allow to debug potential memory issues.
|
Closing this in favor of #13075 13075 |
Superseeds: protocolbuffers#13054 The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory. To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations. Instead we can replace `ObjectCache_Add` by a `getset` type of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance. Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The `ObjectCache` instance is also exposed as `Google::Protobuf::OBJECT_CACHE` to better allow to debug potential memory issues.
5 participants
Fix: #11968
Since calling a method on WeakMap allow the RubyVM to switch threads, adding a non-fully initialized object into the cache allow another thread from using it.
Note: with this fix, the other two thread may create their own wrapper for the same underlying Array. We don't know if it's a big deal or not.
Co-Authored-By: @peterzhu2118
cc @haberman