Skip to content

Commit cf11a13

Browse files
authored
gRPC: in Datastore, delay serializing domain object to a grpc::ByteBuffer right until the call is started (#1949)
(see [here](#1935 (comment)) and [here](grpc/grpc#16875) for context) The problem with serializing a domain object immediately is that the resulting `ByteBuffer` is stored in a `std::function` within Auth. `ByteBuffer`s become invalid once gRPC core shuts down, so if Auth happens to outlive Firestore, once the `ByteBuffer`'s destructor is invoked, the app will crash.
1 parent fedfe66 commit cf11a13

File tree

3 files changed

+16
-19
lines changed

3 files changed

+16
-19
lines changed

Firestore/core/src/firebase/firestore/remote/datastore.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -120,14 +120,14 @@ class Datastore : public std::enable_shared_from_this<Datastore> {
120120
void PollGrpcQueue();
121121

122122
void CommitMutationsWithCredentials(const auth::Token& token,
123-
const grpc::ByteBuffer& message,
123+
NSArray<FSTMutation*>* mutations,
124124
FSTVoidErrorBlock completion);
125125
void OnCommitMutationsResponse(const util::StatusOr<grpc::ByteBuffer>& result,
126126
FSTVoidErrorBlock completion);
127127

128128
void LookupDocumentsWithCredentials(
129129
const auth::Token& token,
130-
const grpc::ByteBuffer& message,
130+
const std::vector<model::DocumentKey>& keys,
131131
FSTVoidMaybeDocumentArrayErrorBlock completion);
132132
void OnLookupDocumentsResponse(
133133
const util::StatusOr<std::vector<grpc::ByteBuffer>>& result,

Firestore/core/src/firebase/firestore/remote/datastore.mm

+13-13
Original file line numberDiff line numberDiff line change
@@ -151,23 +151,23 @@ void LogGrpcCallFinished(absl::string_view rpc_name,
151151

152152
void Datastore::CommitMutations(NSArray<FSTMutation*>* mutations,
153153
FSTVoidErrorBlock completion) {
154-
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
155-
serializer_bridge_.CreateCommitRequest(mutations));
156-
157154
ResumeRpcWithCredentials(
158-
[this, message, completion](const StatusOr<Token>& maybe_credentials) {
155+
[this, mutations, completion](const StatusOr<Token>& maybe_credentials) {
159156
if (!maybe_credentials.ok()) {
160157
completion(util::MakeNSError(maybe_credentials.status()));
161158
return;
162159
}
163-
CommitMutationsWithCredentials(maybe_credentials.ValueOrDie(), message,
164-
completion);
160+
CommitMutationsWithCredentials(maybe_credentials.ValueOrDie(),
161+
mutations, completion);
165162
});
166163
}
167164

168165
void Datastore::CommitMutationsWithCredentials(const Token& token,
169-
const grpc::ByteBuffer& message,
166+
NSArray<FSTMutation*>* mutations,
170167
FSTVoidErrorBlock completion) {
168+
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
169+
serializer_bridge_.CreateCommitRequest(mutations));
170+
171171
std::unique_ptr<GrpcUnaryCall> call_owning = grpc_connection_.CreateUnaryCall(
172172
kRpcNameCommit, token, std::move(message));
173173
GrpcUnaryCall* call = call_owning.get();
@@ -196,24 +196,24 @@ void LogGrpcCallFinished(absl::string_view rpc_name,
196196
void Datastore::LookupDocuments(
197197
const std::vector<DocumentKey>& keys,
198198
FSTVoidMaybeDocumentArrayErrorBlock completion) {
199-
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
200-
serializer_bridge_.CreateLookupRequest(keys));
201-
202199
ResumeRpcWithCredentials(
203-
[this, message, completion](const StatusOr<Token>& maybe_credentials) {
200+
[this, keys, completion](const StatusOr<Token>& maybe_credentials) {
204201
if (!maybe_credentials.ok()) {
205202
completion(nil, util::MakeNSError(maybe_credentials.status()));
206203
return;
207204
}
208-
LookupDocumentsWithCredentials(maybe_credentials.ValueOrDie(), message,
205+
LookupDocumentsWithCredentials(maybe_credentials.ValueOrDie(), keys,
209206
completion);
210207
});
211208
}
212209

213210
void Datastore::LookupDocumentsWithCredentials(
214211
const Token& token,
215-
const grpc::ByteBuffer& message,
212+
const std::vector<DocumentKey>& keys,
216213
FSTVoidMaybeDocumentArrayErrorBlock completion) {
214+
grpc::ByteBuffer message = serializer_bridge_.ToByteBuffer(
215+
serializer_bridge_.CreateLookupRequest(keys));
216+
217217
std::unique_ptr<GrpcStreamingReader> call_owning =
218218
grpc_connection_.CreateStreamingReader(kRpcNameLookup, token,
219219
std::move(message));

Firestore/core/test/firebase/firestore/remote/datastore_test.mm

+1-4
Original file line numberDiff line numberDiff line change
@@ -340,10 +340,7 @@ void ForceFinishAnyTypeOrder(
340340
EXPECT_NO_THROW(credentials.InvokeGetToken());
341341
}
342342

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

349346
worker_queue.EnqueueBlocking([&] {

0 commit comments

Comments
 (0)