gRPC: in Datastore, delay serializing domain object to a grpc::ByteBuffer right until the call is started
#1949
Conversation
| ResumeRpcWithCredentials( | ||
| [this, message, completion](const StatusOr<Token>& maybe_credentials) { | ||
| [this, mutations, completion](const StatusOr<Token>& maybe_credentials) { |
There was a problem hiding this comment.
I'm not sure how this works with objc++...
Your lambda is capturing the mutations pointer and then using it later. Is it possible that the thing the pointer points to becomes invalid and/or something else by the time the lambda is actually executed?
There was a problem hiding this comment.
My understanding is that ARC makes all Objective-C pointers behave similarly to std::shared_ptr.
There was a problem hiding this comment.
So it should still exist due to arc, though may have been mutated. But I think the FSTMutation instances are (somewhat ironically) immutable. And I don't believe we change the array itself either, so this seems reasonable.
It may be worth a quick docstring in the header to indicate that the nsarray shouldn't be changed?
There was a problem hiding this comment.
Hmm, if there's a concern regarding mutation of the array, I'd rather make a protective copy. Unfortunately, I'm not sure what is the normal practice in Objective-C regarding this. @wilhuff, can you please take a look at this discussion thread? Since the given NSArray is stored for future use after the function is done, would it make sense to store a copy of the array?
There was a problem hiding this comment.
In the Objective-C style guide it's specifically called out that if you're going to retain an NSString for a while you should make your own copy, e.g. via [foo copy]. If foo is actually an NSMutableString, this will create a copy, but otherwise this is a no-op.
This advice does not extend to other types, even if they implement NSCopying. Conventionally for things like arrays, it's assumed that if you're giving away your pointer to it, it's no longer modifiable.
If you want to be particularly careful about this you could make this take a std::vector<FSTMutation*>&& which would require the caller to explicitly give the mutations away. We eventually want to move to C++ anyway so moving to C++ containers before their contents can be moved is reasonable.
In any case, I don't think it's worth stressing about this. Documentation on this point isn't going to be read and the defensive copy is more expensive than it's worth.
There was a problem hiding this comment.
Just throw in my 1 cent. As far as this works for iOS client, I am fine with this As Is. Whether CMake build actually enabled arc for object c object I am not sure; but if the goal is to make this work for the iOS client SDK, I'd make this logic as simple as possible, which is to rely on the ARC as most of our current iOS object c code does. Later when we are ready to deploy the code to other platform, we can revisited this (actually, then the mutation will be in C++ instead of being FSTMutation, and thus there may not be any issue by then).
There was a problem hiding this comment.
I'm fine with it as is. PR still lgtm.
| FSTVoidErrorBlock completion) { | ||
| grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer( |
There was a problem hiding this comment.
If grpc has already shut down at this point, what does this do? (Or is that even possible to get here if grpc has already shut down?)
There was a problem hiding this comment.
gRPC isn't shut down until Datastore is shut down, and this check should prevent this function from being invoked if Datastore is shut down.
| FSTVoidErrorBlock completion) { | ||
| grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer( |
| ResumeRpcWithCredentials( | ||
| [this, message, completion](const StatusOr<Token>& maybe_credentials) { | ||
| [this, mutations, completion](const StatusOr<Token>& maybe_credentials) { |
There was a problem hiding this comment.
So it should still exist due to arc, though may have been mutated. But I think the FSTMutation instances are (somewhat ironically) immutable. And I don't believe we change the array itself either, so this seems reasonable.
It may be worth a quick docstring in the header to indicate that the nsarray shouldn't be changed?
| ResumeRpcWithCredentials( | ||
| [this, message, completion](const StatusOr<Token>& maybe_credentials) { | ||
| [this, mutations, completion](const StatusOr<Token>& maybe_credentials) { |
There was a problem hiding this comment.
Just throw in my 1 cent. As far as this works for iOS client, I am fine with this As Is. Whether CMake build actually enabled arc for object c object I am not sure; but if the goal is to make this work for the iOS client SDK, I'd make this logic as simple as possible, which is to rely on the ARC as most of our current iOS object c code does. Later when we are ready to deploy the code to other platform, we can revisited this (actually, then the mutation will be in C++ instead of being FSTMutation, and thus there may not be any issue by then).
(see here and here for context)
The problem with serializing a domain object immediately is that the resulting
ByteBufferis stored in astd::functionwithin Auth.ByteBuffers become invalid once gRPC core shuts down, so if Auth happens to outlive Firestore, once theByteBuffer's destructor is invoked, the app will crash.