Skip to content

Simplify Serializer [En|De]code*() methods to no longer [de]serialize manually. #1816

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 20 commits into from
Oct 19, 2018
Merged

Simplify Serializer [En|De]code*() methods to no longer [de]serialize manually. #1816

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
16b5c07
Simplify Serializer Decode*() methods to no longer deserialize manually.
rsgowman Sep 11, 2018
3cff51f
Enable nanopb PB_ENABLE_MALLOC
rsgowman Sep 20, 2018
45ef270
Simplify Serializer Encode*() methods to no longer serialize manually.
rsgowman Sep 26, 2018
3e47d0a
Remove obsolete methods from nanopb::Writer
rsgowman Sep 26, 2018
c464a8c
Remove obsolete methods from nanopb::Reader
rsgowman Sep 26, 2018
ec318ba
Remove obsolete nanopb::Tag
rsgowman Sep 26, 2018
f2f4c17
Remove optional<> from serializer return values
rsgowman Oct 2, 2018
51eb58e
Review feedback: reinterpret -> static cast
rsgowman Oct 4, 2018
882cf0b
Remove unused variable
rsgowman Oct 15, 2018
b900930
Integrate into xcode build
rsgowman Oct 16, 2018
9c06c44
Review feedback: MakeArray
rsgowman Oct 16, 2018
437b9b0
Review feedback: Use c++ initializers to 0 memory
rsgowman Oct 16, 2018
12ce994
Review feedback: fix error message
rsgowman Oct 17, 2018
8414576
style.sh
rsgowman Oct 17, 2018
56c0dcd
Review feedback: rm now low value comments
rsgowman Oct 18, 2018
2b0d300
Change version of nanopb pod without actually changing the version.
rsgowman Oct 19, 2018
3a3acc7
Review feedback: assert->fail + eliminate early returns
rsgowman Oct 19, 2018
b6e568d
./style.sh
rsgowman Oct 19, 2018
30cecdf
Review feedback: brace initialization
rsgowman Oct 19, 2018
f2d0037
Merge remote-tracking branch 'origin/master' into rsgowman/simplify_s…
rsgowman Oct 19, 2018
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
263 changes: 108 additions & 155 deletions Firestore/core/src/firebase/firestore/local/local_serializer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
#include "Firestore/core/src/firebase/firestore/model/field_value.h"
#include "Firestore/core/src/firebase/firestore/model/no_document.h"
#include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
#include "Firestore/core/src/firebase/firestore/nanopb/tag.h"
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
#include "Firestore/core/src/firebase/firestore/util/string_format.h"

namespace firebase {
namespace firestore {
Expand All @@ -41,28 +41,27 @@ using model::NoDocument;
using model::ObjectValue;
using model::SnapshotVersion;
using nanopb::Reader;
using nanopb::Tag;
using nanopb::Writer;
using util::Status;
using util::StringFormat;

firestore_client_MaybeDocument LocalSerializer::EncodeMaybeDocument(
const MaybeDocument& maybe_doc) const {
firestore_client_MaybeDocument result =
firestore_client_MaybeDocument_init_zero;

void LocalSerializer::EncodeMaybeDocument(
Writer* writer, const MaybeDocument& maybe_doc) const {
switch (maybe_doc.type()) {
case MaybeDocument::Type::Document:
writer->WriteTag(
{PB_WT_STRING, firestore_client_MaybeDocument_document_tag});
writer->WriteNestedMessage([&](Writer* writer) {
EncodeDocument(writer, static_cast<const Document&>(maybe_doc));
});
return;
result.which_document_type = firestore_client_MaybeDocument_document_tag;
result.document = EncodeDocument(static_cast<const Document&>(maybe_doc));
return result;

case MaybeDocument::Type::NoDocument:
writer->WriteTag(
{PB_WT_STRING, firestore_client_MaybeDocument_no_document_tag});
writer->WriteNestedMessage([&](Writer* writer) {
EncodeNoDocument(writer, static_cast<const NoDocument&>(maybe_doc));
});
return;
result.which_document_type =
firestore_client_MaybeDocument_no_document_tag;
result.no_document =
EncodeNoDocument(static_cast<const NoDocument&>(maybe_doc));
return result;

case MaybeDocument::Type::Unknown:
// TODO(rsgowman)
Expand All @@ -73,184 +72,138 @@ void LocalSerializer::EncodeMaybeDocument(
}

std::unique_ptr<MaybeDocument> LocalSerializer::DecodeMaybeDocument(
Reader* reader) const {
std::unique_ptr<MaybeDocument> result;

while (reader->good()) {
switch (reader->ReadTag()) {
case firestore_client_MaybeDocument_document_tag:
// TODO(rsgowman): If multiple 'document' values are found, we should
// merge them (rather than using the last one.)
result = reader->ReadNestedMessage<Document>(
rpc_serializer_, &remote::Serializer::DecodeDocument);
break;

case firestore_client_MaybeDocument_no_document_tag:
// TODO(rsgowman): If multiple 'no_document' values are found, we should
// merge them (rather than using the last one.)
result = reader->ReadNestedMessage<NoDocument>(
*this, &LocalSerializer::DecodeNoDocument);
break;

default:
reader->SkipUnknown();
}
}
Reader* reader, const firestore_client_MaybeDocument& proto) const {
if (!reader->status().ok()) return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the error cases in here are now in the realm of vanishingly rare--to the degree that they should basically not happen in production.

So long as the caller handles the failure in some useful way, does it really matter if we do more work than strictly necessary?

I think you should eliminate all these early returns.

Copy link
Member Author

Choose a reason for hiding this comment

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

They protect against hard_asserts; without these early returns, we could end up in a state where our assertions don't hold; this would lead to crashes, rather than returned errors. (That said, we can reduce their number; we really only need them before the hard asserts. Or before functions that call hard_assert)

Alternatively, we could eliminate the hard_asserts and replace them with if (!assertion) return nullptr_or_whatever; But I'd rather crash if our assumptions about the system are wrong.

I looked into moving these checks to be right before the hard_assert statements. It mostly works. But because there are functions that hard_assert, that don't have access to the nanopb reader/writer objects, it doesn't quite. We could add extra plumbing to make that work though. But even then, in this situation, our asserts would be a two step process (check for error; assert;). So still not great. We could refactor that by adding a SOFT_ASSERT that checks the current status code first, and only then calls HARD_ASSERT?

Options:

  1. Remove checks; if a corruption occurs, it'll cause us to crash. (I don't like this; I've been burned by too many flaky drives. I'd prefer to give the developer a chance of noticing and clearing the local cache in an attempt to recover. But this would be a rare occurrence.)
  2. if (!reader->status().ok()) return T; all over the place.
  3. Move checks into SOFT_ASSERT, which would only HARD_ASSERT when there's no error. Add plumbing to make the error accessible everywhere. (Mostly already exists.) Upon corruption (or other error), the HARD_ASSERTS would effectively be disabled, and we'd continue on until eventually returning a nonsense value (which is fine b/c the caller should be checking the status first.)

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

While I consider the goal you're after laudable, I think that in practice this is going to be too hard to get right to make it worth attempting. However, as discussed offline, based on my survey of the HARD_ASSERTS we have we can make simplifications that allow us to have our cake and eat it too.

The hard asserts you're talking about defending against fall into a few broad categories:

  • Assertions about true logic errors that are independent of input, e.g. the one in DecodeMissingDocument. These can remain and don't need any special treatment.
  • Assertions that indicate corruption directly, e.g. the one in DecodeResourceName. If our goal is to avoid crashing on bad input this needs to be changed into a Fail, in which case this no longer needs to be guarded by an early return elsewhere.
  • assertions that indicate corruption indirectly, e.g. the one in DecodeFoundDocument that asserts the the timestamp is non-empty, but one the major way that could have happened is to have had a failure during or prior to reading the timestamp. This should also probably be converted to a failure, though since it can be a secondary failure we should double-check that we don't clobber the root cause.

In any case, after we address this there just won't be many assertions in here so we won't have to keep this extra layer of defense either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Most asserts have been converted to reader->Fail() (which doesn't overwrite the root cause, so we should be good there.)


switch (proto.which_document_type) {
case firestore_client_MaybeDocument_document_tag:
return rpc_serializer_.DecodeDocument(reader, proto.document);

if (!result) {
reader->Fail(
"Invalid MaybeDocument message: Neither 'no_document' nor 'document' "
"fields set.");
return nullptr;
case firestore_client_MaybeDocument_no_document_tag:
return DecodeNoDocument(reader, proto.no_document);

default:
reader->Fail(
"Invalid MaybeDocument message: Neither 'no_document' nor 'document' "
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite true: we've encountered an unknown child of MaybeDocument, which is slightly different than this message. (Also, we could log the value of which_document_type that got us here.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

"fields set.");
return nullptr;
}
return result;

UNREACHABLE();
}

void LocalSerializer::EncodeDocument(Writer* writer,
const Document& doc) const {
google_firestore_v1beta1_Document LocalSerializer::EncodeDocument(
const Document& doc) const {
google_firestore_v1beta1_Document result =
Copy link
Contributor

Choose a reason for hiding this comment

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

You can write this as

google_firestore_v1beta1_Document result{};

C compilers won't generate aggregate initializers for you, so nanopb ships the _init_zero things. In our case in C++ they can mostly be avoided.

Here and throughout.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

google_firestore_v1beta1_Document_init_zero;

// Encode Document.name
writer->WriteTag({PB_WT_STRING, google_firestore_v1beta1_Document_name_tag});
writer->WriteString(rpc_serializer_.EncodeKey(doc.key()));
result.name =
rpc_serializer_.EncodeString(rpc_serializer_.EncodeKey(doc.key()));
Copy link
Contributor

Choose a reason for hiding this comment

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

If EncodeKey returned the nanopb byte array directly, you could eliminate this two-step at every invocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's occasionally used without the EncodeString() surrounding it (though admittedly, mostly in the tests, and even then, only for use with libprotobuf, since it has no knowledge of nanopb byte arrays.) I suppose we could wrap those usages in DecodeString(), or more likely, yet another wrapper to deal with the memory leak.

But it seems simpler to just leave as is for now. I suspect we'll revisit this again when re-evaluating if we should just use nanopb strings for our internal strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I have a mild preference for mirroring the method structure in the other SDKs and pushing testing concerns on the tests, but I'm happy to defer until later as well.


// Encode Document.fields (unless it's empty)
const ObjectValue& object_value = doc.data().object_value();
if (!object_value.internal_value.empty()) {
rpc_serializer_.EncodeObjectMap(
writer, object_value.internal_value,
google_firestore_v1beta1_Document_fields_tag,
google_firestore_v1beta1_Document_FieldsEntry_key_tag,
google_firestore_v1beta1_Document_FieldsEntry_value_tag);
size_t count = doc.data().object_value().internal_value.size();
result.fields_count = count;
result.fields =
reinterpret_cast<google_firestore_v1beta1_Document_FieldsEntry*>(malloc(
Copy link
Contributor

Choose a reason for hiding this comment

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

A parameterized MakeArray function that uses calloc could make this less repetitive:

template <typename T>
T* MakeArray(size_t count) {
  return reinterpret_cast<T*>(calloc(count, sizeof(T)));
}

It cuts down on the actual call site, and eliminates the init fields to zero step in the loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea; done.

sizeof(google_firestore_v1beta1_Document_FieldsEntry) * count));
int i = 0;
for (const auto& kv : doc.data().object_value().internal_value) {
result.fields[i] = google_firestore_v1beta1_Document_FieldsEntry_init_zero;
result.fields[i].key = rpc_serializer_.EncodeString(kv.first);
result.fields[i].value = rpc_serializer_.EncodeFieldValue(kv.second);
i++;
}

// Encode Document.update_time
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments made considerably more sense when it took 3-5 lines of non-obvious code for each field, but now that this is a simple assignment, maybe we can skip them? Here and throughout.

In this specific case, if you thought it worthwhile you could combine it with the comments below to draw a contrast. Something like, "Encode update_time but not create_time because we don't use the latter in our on-disk protos." I'm also happy to leave the comment below alone (especially if that's what's on the other platforms).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've left the below comment, but have removed this one, and all the other now-obvious ones throughout local/remote serializers.

writer->WriteTag(
{PB_WT_STRING, google_firestore_v1beta1_Document_update_time_tag});
writer->WriteNestedMessage([&](Writer* writer) {
rpc_serializer_.EncodeVersion(writer, doc.version());
});
result.update_time = rpc_serializer_.EncodeVersion(doc.version());

// Ignore Document.create_time. (We don't use this in our on-disk protos.)

return result;
}

void LocalSerializer::EncodeNoDocument(Writer* writer,
const NoDocument& no_doc) const {
firestore_client_NoDocument LocalSerializer::EncodeNoDocument(
const NoDocument& no_doc) const {
firestore_client_NoDocument result = firestore_client_NoDocument_init_zero;

// Encode NoDocument.name
writer->WriteTag({PB_WT_STRING, firestore_client_NoDocument_name_tag});
writer->WriteString(rpc_serializer_.EncodeKey(no_doc.key()));
result.name =
rpc_serializer_.EncodeString(rpc_serializer_.EncodeKey(no_doc.key()));

// Encode NoDocument.read_time
writer->WriteTag({PB_WT_STRING, firestore_client_NoDocument_read_time_tag});
writer->WriteNestedMessage([&](Writer* writer) {
rpc_serializer_.EncodeVersion(writer, no_doc.version());
});
result.read_time = rpc_serializer_.EncodeVersion(no_doc.version());

return result;
}

std::unique_ptr<NoDocument> LocalSerializer::DecodeNoDocument(
Reader* reader) const {
std::string name;
absl::optional<SnapshotVersion> version = SnapshotVersion::None();

while (reader->good()) {
switch (reader->ReadTag()) {
case firestore_client_NoDocument_name_tag:
name = reader->ReadString();
break;

case firestore_client_NoDocument_read_time_tag:
version = reader->ReadNestedMessage<SnapshotVersion>(
rpc_serializer_.DecodeSnapshotVersion);
break;

default:
reader->SkipUnknown();
break;
}
}
Reader* reader, const firestore_client_NoDocument& proto) const {
if (!reader->status().ok()) return nullptr;

SnapshotVersion version =
rpc_serializer_.DecodeSnapshotVersion(reader, proto.read_time);

if (!reader->status().ok()) return nullptr;
return absl::make_unique<NoDocument>(rpc_serializer_.DecodeKey(name),
*std::move(version));
return absl::make_unique<NoDocument>(
rpc_serializer_.DecodeKey(rpc_serializer_.DecodeString(proto.name)),
Copy link
Contributor

Choose a reason for hiding this comment

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

These decodes can conceivably fail and you're doing them after the status check above which will result in an invalid NoDocument.

Somewhat counter-intuitively, I think the fix is to remove the status check. Even if we try to prevent the creation of invalid objects, we should assume we're going to get it wrong: we're going to end up creating bogus instances in the error case anyway because it's hard to verify we're doing this right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Invalid docs are fine; I'm not trying to avoid that. The check is to prevent hard_assert'ing if there's a corruption. I think ideally, we'd push the check into DecodeKey(), etc though. If you assume DecodeString() fails, and could cause a hard assert in DecodeKey(), then the existing check above isn't going to be sufficient. (DecodeString() can't actually fail right now.)

std::move(version));
}

void LocalSerializer::EncodeQueryData(Writer* writer,
const QueryData& query_data) const {
writer->WriteTag({PB_WT_VARINT, firestore_client_Target_target_id_tag});
writer->WriteInteger(query_data.target_id());

writer->WriteTag(
{PB_WT_STRING, firestore_client_Target_snapshot_version_tag});
writer->WriteNestedMessage([&](Writer* writer) {
rpc_serializer_.EncodeTimestamp(writer,
query_data.snapshot_version().timestamp());
});
firestore_client_Target LocalSerializer::EncodeQueryData(
const QueryData& query_data) const {
firestore_client_Target result = firestore_client_Target_init_zero;

writer->WriteTag({PB_WT_STRING, firestore_client_Target_resume_token_tag});
writer->WriteBytes(query_data.resume_token());
result.target_id = query_data.target_id();
result.snapshot_version = rpc_serializer_.EncodeTimestamp(
query_data.snapshot_version().timestamp());
result.resume_token = rpc_serializer_.EncodeBytes(query_data.resume_token());
Copy link
Contributor

@wilhuff wilhuff Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think there's still a place for nanopb::Writer in our system. If we call pb_release on messages once we're done encoding to bytes, nanopb is going to free every pointer in there. This means that we must copy everything we put in the message just so pb_release doesn't destroy objects we're otherwise holding onto.

However, if we tracked every malloc separately, we could freely mingle model values and malloc'ed objects without fear of nanopb freeing a value we want to retain elsewhere. That tracking has a natural home in the Writer.

Note that tracking shared objects (with the intent of nulling those out before calling pb_release) is harder to do because we encode into objects that allow moves. We could traverse the message to find these (using the field metadata nanopb provides), but this is fragile both because it depends on nanopb implementation and because it relies on us remembering to track a value we share. Tracking allocations is easier because it's fully under our control and is automatic.

On the reading side we can't take the same approach, since nanopb is doing the mallocing. There we're going to need to pb_release. We can still share values there by taking them from the message and nulling out the source. That way when pb_release comes along there will be a null-value there and free will do nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, yeah we could track that in the writer. At this point, it isn't doing much else. :)


const Query& query = query_data.query();
if (query.IsDocumentQuery()) {
// TODO(rsgowman): Implement. Probably like this (once EncodeDocumentsTarget
// exists):
/*
writer->WriteTag({PB_WT_STRING, firestore_client_Target_documents_tag});
writer->WriteNestedMessage([&](Writer* writer) {
rpc_serializer_.EncodeDocumentsTarget(writer, query);
});
result.which_target_type = firestore_client_Target_document_tag;
result.documents = rpc_serializer_.EncodeDocumentsTarget(query);
*/
abort();
} else {
writer->WriteTag({PB_WT_STRING, firestore_client_Target_query_tag});
writer->WriteNestedMessage([&](Writer* writer) {
rpc_serializer_.EncodeQueryTarget(writer, query);
});
result.which_target_type = firestore_client_Target_query_tag;
result.query = rpc_serializer_.EncodeQueryTarget(query);
}

return result;
}

absl::optional<QueryData> LocalSerializer::DecodeQueryData(
Reader* reader) const {
model::TargetId target_id = 0;
absl::optional<SnapshotVersion> version = SnapshotVersion::None();
std::vector<uint8_t> resume_token;
absl::optional<Query> query = Query::Invalid();

while (reader->good()) {
switch (reader->ReadTag()) {
case firestore_client_Target_target_id_tag:
// TODO(rsgowman): How to handle truncation of integer types?
target_id = static_cast<model::TargetId>(reader->ReadInteger());
break;

case firestore_client_Target_snapshot_version_tag:
version = reader->ReadNestedMessage<SnapshotVersion>(
rpc_serializer_.DecodeSnapshotVersion);
break;

case firestore_client_Target_resume_token_tag:
resume_token = reader->ReadBytes();
break;

case firestore_client_Target_query_tag:
// TODO(rsgowman): Clear 'documents' field (since query and documents
// are part of a 'oneof').
query =
reader->ReadNestedMessage<Query>(rpc_serializer_.DecodeQueryTarget);
break;

case firestore_client_Target_documents_tag:
// Clear 'query' field (since query and documents are part of a 'oneof')
query = Query::Invalid();
// TODO(rsgowman): Implement.
abort();

default:
reader->SkipUnknown();
break;
}
QueryData LocalSerializer::DecodeQueryData(
Reader* reader, const firestore_client_Target& proto) const {
if (!reader->status().ok()) return QueryData::Invalid();

model::TargetId target_id = proto.target_id;
SnapshotVersion version =
rpc_serializer_.DecodeSnapshotVersion(reader, proto.snapshot_version);
std::vector<uint8_t> resume_token =
rpc_serializer_.DecodeBytes(proto.resume_token);
Query query = Query::Invalid();

switch (proto.which_target_type) {
case firestore_client_Target_query_tag:
query = rpc_serializer_.DecodeQueryTarget(reader, proto.query);
break;

case firestore_client_Target_documents_tag:
// TODO(rsgowman): Implement.
abort();

default:
reader->Fail(
StringFormat("Unknown target_type: %s", proto.which_target_type));
}

if (!reader->status().ok()) return absl::nullopt;
return QueryData(*std::move(query), target_id, QueryPurpose::kListen,
*std::move(version), std::move(resume_token));
if (!reader->status().ok()) return QueryData::Invalid();
return QueryData(std::move(query), target_id, QueryPurpose::kListen,
std::move(version), std::move(resume_token));
}

} // namespace local
Expand Down
Loading