Serialize (and deserialize) string values

[rsgowman]

No description provided.

[var-const]

LGTM with small nits.

[var-const]

Typo: s/pb_clost/pb_close

[rsgowman]

Done

[var-const]

Optional nit: maybe do a helper function like handle_error() { /* TODO */ abort(); } or validate_status(bool status, SomeType error) { /*TODO*/ if (!status) abort; } to avoid copying the same comment around? This presumes that error handling will be pretty similar across {en,de}coders.

[rsgowman]

Rather than do all that refactoring, I'd probably be better off to just implement error handling; it really isn't going to be all that complicated. I'll issue a followup PR.

[var-const]

Nit: feel free to disagree, but I don't think this is magic. It's valid to embed a universal character name inside a char string literal, it will map to several characters if necessary, so I'm not sure if there's any narrowing involved. Consider rephrasing to something like:

"Each one of the three embedded universal character names (\u-escaped) maps to three chars, so the total length of the string literal is 10 (ignoring the terminating null), and the resulting string literal is the same as '\0\xed\x9f\xbf\xee\x80\x80\xef\xbf\xbf'"

or similar.

[rsgowman]

I like your phrasing better. (Done) The thing that confused me initially was the \0 prefix. I didn't look close enough to realize that's a null, rather than just another unicode escape character, and was surprised when my encoding function encoded 0 bytes. :)

[var-const]

Ah, yeah, std::string can contain embedded nulls, which can be a gotcha when using c_str.

[rsgowman]

In my case, it was actually the other way around. i.e. std::string("\0ABCDEFG") contains an empty string and doesn't have any embedded nulls in it. (The ctor stops processing the input char* as soon as it sees the null at position 0... unless you also indicate the size.)

[var-const]

Right, that constructor presumes a null-terminated string, because there's no way to figure out the role of a given null. I had a funny quirk reimplementing some Objective C parsing in C++: when parsing an std::string, I had to make sure parsing stops upon encountering '\0' even if it comes before the last character in the string is reached, for consistency with existing behavior (this might be a little paranoid, though).

[zxu123]

LGTM

[zxu123]

absolutely optional: Is it possible to use escape-sequence for the unicode character here instead of use that character directly? Assuming any modern editor/source-control-system supports unicode, there is no problem; so it is optional.

[rsgowman]

We could; but OTOH, we've already got some test cases that use escape characters below. (More context: These test cases are taken from the other test suites, so my preference is to leave them as close as possible to the original.)

We could make an argument that this is redundant... but it's probably more trouble than it's worth to remove it across all platforms.

[wilhuff]

Pretty much LGTM

[wilhuff]

This should be in a separate group from C headers (scripts/lint.sh should have flagged).

[rsgowman]

Fixed.

lint.sh did not catch this (on either mac or linux)

[wilhuff]

I really hate to ask this question but does this need to be exception safe?

As it stands the string allocation could fail and we'd fail to undo this business here.

A simple way forward would be to move the allocation of result outside the group of C function calls. Secondarily though, there's the question of when we adopt error handling, is it a leak to fail to call this?

[var-const]

@wilhuff I thought we just crash on exceptions and basically treat them as if they didn't exist?

[wilhuff]

In Objective-C, yes. Exceptions aren't expected to be handled and the runtime isn't capable of handling them without leaking anyway.

In general we don't throw exceptions there except for assertions and we expect those to crash the app.

In plain C++ that's not necessarily the case. Does anyone reasonably catch std::bad_alloc?

[var-const]

I mean in C++. The style guide doesn't seem to distinguish between try and catch, so I presumed we don't use either. I'd be happy if that's not the case.

[var-const]

Ah, I see, you mean user code should be able to catch the exception and Firestore shouldn't leak. If that's the case, sorry for misunderstanding.

Does anyone reasonably catch std::bad_alloc?

Can't think of how client code might reasonably expect to catch it from here.

[rsgowman]

I really hate to ask this question

You've just kicked the hornet's nest. :(

A simple way forward would be to move the allocation of result outside the group of C function calls

I can't easily[1] do that. I need to wait until after pb_make_string_substream to know how large the string should be. (Well, I could allocate 0 bytes, and then extend it afterwards, but that doesn't avoid the problem.)

[1] - not technically correct. I could peak into the implementation details of pb_make_string_substream and simply do what it does, namely read the size required from the input stream. At which point, I've essentially just inlined the function.

does this need to be exception safe?

In this particular case, I believe the code is safe.

pb_make_string_substream/pb_close_string_substream don't actually allocate any memory. In particular, the close call just sets state on the stream (and newer versions of nanopb also ensure the substream is "consumed", i.e. the pointer is pointing to the end rather than somewhere in the middle.) So generally, there's no leak.

However, the stream may not be consistent, so recovering from bad_alloc and assuming the stream is fine would be incorrect. So as long as we don't catch the exception, we're ok (since by the time the exception goes up to the user, the stream has fallen off the stack. Also note that streams are typically stack allocated, so no free() calls are required.)

That said, these calls look like they could malloc or open() or otherwise consume resources that needs to be freed and, while nanopb clearly tries really hard to not malloc(), there's no guarantee it wouldn't change the implementation in the future to do this. (Side note: if you define PB_ENABLE_MALLOC=0 while compiling nanopb, it will ifdef out all the malloc calls. This reduces functionality, but a quick glance didn't turn up anything we actually use, so this might be worth investigating. Worst case: it would fail and therefore confirm that we are actually mallocing within nanopb.)

Does anyone reasonably catch std::bad_alloc?

Not generally on linux. These don't occur by default on linux due to it's over-committing memory model. (Instead, it relies on the infamous OOM killer to "take care" of things.) But I don't know about android/ios. (Probably developers still don't since there's usually not much that can usefully be done except crash.) OTOH, when people actually do catch bad_alloc, it's safe to assume that they really, really care about memory management, and would likely be disappointed (though not surprised) when a library they used didn't clean up properly due to this state.

So, options:

1. noop. I think we're in a safe situation right now, even if bad_alloc is thrown. (More generally, we could always ignore bad_alloc and hope no one cares about this state.)
2. catch bad_alloc (or ...). Then rethrow. (We don't use exceptions. But can we catch/rethrow them? I think we could add a catch block which would simply be not evaluated if -fno-exceptions is defined. In this situation, I think std::string would simply abort.)
3. similar to 2; use char* buffer = malloc(size) and then check for null. (And then remember to free() after)
4. similar to 2; but use #ifdef __cpp_exceptions.
5. inline the nanopb calls. (Not a fan of this one.)
6. wrap stuff like this in a c++ object with a dtor that handles cleanup. (i.e. RAII-ify nanopb)
7. provide a different allocator to std::string which never throws, even when exceptions are enabled. (Haven't investigated this, but believe it to be feasible?)

Votes? (Keep in mind this will likely appear in many other places too, so vote twice if you feel we should do something different here than the general guidance.)

I like (1) for this specific case, and (2), (6) or (7) for the general case. (But (1) for the general case may be more pragmatic.)

[wilhuff]

Notes:

1. Seems good in this instance
2. is not viable:

   int main() {
     int* foo = nullptr;
     try {
       foo = new int(42);
     } catch (...) {
     }
     return *foo;
   }

   Fails to compile:

   # clang++ -fno-exceptions -std=c++11 test.cc
   test.cc:3:3: error: cannot use 'try' with exceptions disabled
     try {
     ^
   1 error generated.
   

3. I'm not super happy about this as an approach just to be exception safe. Prefer (7) if at all.
4. __cpp_exceptions is insufficient for all compilers. Thankfully ABSL_HAVE_EXCEPTIONS does the right thing. I'd consider this unusable if we had to do this in every function that could potentially throw though.
5. Agreed
6. For places it really matters and it's an error we're actually going to handle and continue processing then thisis the way to go. For nanopb specifically, I think any error returned here indicates we made a programming error so this should ultimately end up in an assertion failure.
7. new (std::nothrow) char[bufsize] from #include <new>.

I'd articulate general principles:

• In cases where we have heavy weight objects we need to remember to deallocate we should RAII just for our own sanity. At the very least with std::unique_ptr.
• Avoid malloc/free unless specifically dealing with a C API that requires it (e.g. asprintf if we were crazy enough to use it).
• Even still, we might consider using unique_ptr so we can't forget, e.g.

  auto buf = std::unique_ptr<char, decltype(free)*>(reinterpret_cast<char*>(malloc(42)), free);

• We tolerate that libraries we use might throw but don't attempt to recover.
• We don't throw our own C++ exceptions
• If we could usefully recover wrap the invocation in a lambda and pass that to a helper that does the #ifdef magic. This will be an extremely high bar to get over and must demonstrably and meaningfully improve developer quality of life.

If we agree I'll chuck these I'll chuck these in our migration guide (plus your feedback of course).

[var-const]

If we're voting, I'd vote for 1 and 6, depending on the situation.

[rsgowman]

Re (2): Darn. For some reason, I thought that worked. (Must've been confused.)

Yeah, everything else sgtm. I'll merge this PR shortly as is.

[wilhuff]

LGTM

[wilhuff]

Notes:

1. Seems good in this instance
2. is not viable:

   int main() {
     int* foo = nullptr;
     try {
       foo = new int(42);
     } catch (...) {
     }
     return *foo;
   }

   Fails to compile:

   # clang++ -fno-exceptions -std=c++11 test.cc
   test.cc:3:3: error: cannot use 'try' with exceptions disabled
     try {
     ^
   1 error generated.
   

3. I'm not super happy about this as an approach just to be exception safe. Prefer (7) if at all.
4. __cpp_exceptions is insufficient for all compilers. Thankfully ABSL_HAVE_EXCEPTIONS does the right thing. I'd consider this unusable if we had to do this in every function that could potentially throw though.
5. Agreed
6. For places it really matters and it's an error we're actually going to handle and continue processing then thisis the way to go. For nanopb specifically, I think any error returned here indicates we made a programming error so this should ultimately end up in an assertion failure.
7. new (std::nothrow) char[bufsize] from #include <new>.

I'd articulate general principles:

• In cases where we have heavy weight objects we need to remember to deallocate we should RAII just for our own sanity. At the very least with std::unique_ptr.
• Avoid malloc/free unless specifically dealing with a C API that requires it (e.g. asprintf if we were crazy enough to use it).
• Even still, we might consider using unique_ptr so we can't forget, e.g.

  auto buf = std::unique_ptr<char, decltype(free)*>(reinterpret_cast<char*>(malloc(42)), free);

• We tolerate that libraries we use might throw but don't attempt to recover.
• We don't throw our own C++ exceptions
• If we could usefully recover wrap the invocation in a lambda and pass that to a helper that does the #ifdef magic. This will be an extremely high bar to get over and must demonstrably and meaningfully improve developer quality of life.

If we agree I'll chuck these I'll chuck these in our migration guide (plus your feedback of course).