Skip to content

gRPC: in Datastore, delay serializing domain object to a grpc::ByteBuffer right until the call is started #1949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 17, 2018
Merged

gRPC: in Datastore, delay serializing domain object to a grpc::ByteBuffer right until the call is started #1949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Firestore/core/src/firebase/firestore/remote/datastore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,14 @@ class Datastore : public std::enable_shared_from_this<Datastore> {
void PollGrpcQueue();

void CommitMutationsWithCredentials(const auth::Token& token,
const grpc::ByteBuffer& message,
NSArray<FSTMutation*>* mutations,
FSTVoidErrorBlock completion);
void OnCommitMutationsResponse(const util::StatusOr<grpc::ByteBuffer>& result,
FSTVoidErrorBlock completion);

void LookupDocumentsWithCredentials(
const auth::Token& token,
const grpc::ByteBuffer& message,
const std::vector<model::DocumentKey>& keys,
FSTVoidMaybeDocumentArrayErrorBlock completion);
void OnLookupDocumentsResponse(
const util::StatusOr<std::vector<grpc::ByteBuffer>>& result,
Expand Down
26 changes: 13 additions & 13 deletions Firestore/core/src/firebase/firestore/remote/datastore.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,23 +148,23 @@ void LogGrpcCallFinished(absl::string_view rpc_name,

void Datastore::CommitMutations(NSArray<FSTMutation*>* mutations,
FSTVoidErrorBlock completion) {
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
serializer_bridge_.CreateCommitRequest(mutations));

ResumeRpcWithCredentials(
[this, message, completion](const StatusOr<Token>& maybe_credentials) {
[this, mutations, completion](const StatusOr<Token>& maybe_credentials) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that ARC makes all Objective-C pointers behave similarly to std::shared_ptr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilhuff Thanks for explaining.

@rsgowman All in all, I'm inclined to consider this not worth handling explicitly (this function only has one caller, after all), but I don't feel strongly about it. I can add a comment if you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with it as is. PR still lgtm.

if (!maybe_credentials.ok()) {
completion(util::MakeNSError(maybe_credentials.status()));
return;
}
CommitMutationsWithCredentials(maybe_credentials.ValueOrDie(), message,
completion);
CommitMutationsWithCredentials(maybe_credentials.ValueOrDie(),
mutations, completion);
});
}

void Datastore::CommitMutationsWithCredentials(const Token& token,
const grpc::ByteBuffer& message,
NSArray<FSTMutation*>* mutations,
FSTVoidErrorBlock completion) {
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, excellent. lgtm.

serializer_bridge_.CreateCommitRequest(mutations));

std::unique_ptr<GrpcUnaryCall> call_owning = grpc_connection_.CreateUnaryCall(
kRpcNameCommit, token, std::move(message));
GrpcUnaryCall* call = call_owning.get();
Expand Down Expand Up @@ -193,24 +193,24 @@ void LogGrpcCallFinished(absl::string_view rpc_name,
void Datastore::LookupDocuments(
const std::vector<DocumentKey>& keys,
FSTVoidMaybeDocumentArrayErrorBlock completion) {
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
serializer_bridge_.CreateLookupRequest(keys));

ResumeRpcWithCredentials(
[this, message, completion](const StatusOr<Token>& maybe_credentials) {
[this, keys, completion](const StatusOr<Token>& maybe_credentials) {
if (!maybe_credentials.ok()) {
completion(nil, util::MakeNSError(maybe_credentials.status()));
return;
}
LookupDocumentsWithCredentials(maybe_credentials.ValueOrDie(), message,
LookupDocumentsWithCredentials(maybe_credentials.ValueOrDie(), keys,
completion);
});
}

void Datastore::LookupDocumentsWithCredentials(
const Token& token,
const grpc::ByteBuffer& message,
const std::vector<DocumentKey>& keys,
FSTVoidMaybeDocumentArrayErrorBlock completion) {
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
serializer_bridge_.CreateLookupRequest(keys));

std::unique_ptr<GrpcStreamingReader> call_owning =
grpc_connection_.CreateStreamingReader(kRpcNameLookup, token,
std::move(message));
Expand Down
5 changes: 1 addition & 4 deletions Firestore/core/test/firebase/firestore/remote/datastore_test.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,7 @@ void Shutdown() {
EXPECT_NO_THROW(credentials.InvokeGetToken());
}

// TODO(varconst): this test currently fails due to a gRPC issue, see here
// https://github.com/firebase/firebase-ios-sdk/pull/1935#discussion_r224900667
// for details. Reenable when/if possible.
TEST_F(DatastoreTest, DISABLED_AuthOutlivesDatastore) {
TEST_F(DatastoreTest, AuthOutlivesDatastore) {
credentials.DelayGetToken();

worker_queue.EnqueueBlocking([&] {
Expand Down