Deep-copy the process bundle descriptor when creating a new bundle processor. #29742
Conversation
|
R: @scwhittle |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
| # Reduce risks of concurrent modifications of the same protos | ||
| # captured in bundle descriptor when the same bundle desciptor is used | ||
| # in different instructions. | ||
| copy.deepcopy(self.fns[bundle_descriptor_id]), | ||
| self.state_handler_factory.create_state_handler( | ||
| self.fns[bundle_descriptor_id].state_api_service_descriptor), |
There was a problem hiding this comment.
use the deepcopied descriptor_id here too?
There was a problem hiding this comment.
sure, that shouldn't hurt and would be cleaner.
| # Reduce risks of concurrent modifications of the same protos | ||
| # captured in bundle descriptor when the same bundle desciptor is used | ||
| # in different instructions. | ||
| copy.deepcopy(self.fns[bundle_descriptor_id]), |
There was a problem hiding this comment.
Not sure if it would be safer to use Message generated CopyFrom method instead. Unclear to me if deepcopy would do something odd with lower-level upb impl.
deepcopy isn't mentioned here https://protobuf.dev/reference/python/python-generated/
There was a problem hiding this comment.
deepcopy (impl: https://github.com/python/cpython/blob/9263173280d7ce949911965efa5b745287c581b2/Lib/copy.py#L118) should defer to __deepcopy__ https://github.com/search?q=repo%3Aprotocolbuffers%2Fprotobuf%20%22__deepcopy__%22&type=code , which calls MergeFrom; CopyFrom also uses MergeFrom: https://github.com/protocolbuffers/protobuf/blob/5076e6206e327942630e82bcec145826cf380559/python/google/protobuf/message.py#L91, so yes we can copy ourselves and save some cycles that perform all the branching.
…scriptor on the clone.
|
thanks, @scwhittle , PTAL. |
fingers crossed but early results are encouraging. |
After protobuf v4 upgrade we are seeing SDK crashes in #27330 .
A possible reason might be that micro-protobuf protos objects are more sensitive to unsafe concurrent modifications, leading to process memory corruption.
This change should reduce the risk of concurrent modifications.