abort() in libprotobuf due to missing required field

[Dave Bailey]

Hi,

libprotobuf calls abort() if one attempts to serialize a message that
is missing a required field.

To me, this seems excessive, because it gives the calling code no
opportunity to handle the error in a different way.

Here is the bug report in protobuf-perlxs:

http://code.google.com/p/protobuf-perlxs/issues/detail?id=5

I have to do a lot of gymnastics to "catch" an abort(). Is it
possible to have libprotobuf throw an exception instead, or at least
give protoc (via an option of some sort) the ability to generate code
that would cause libprotobuf throw an exception if the caller tries to
serialize a message with a missing required field?

This would make it possible to use the traditional Perl method of
trapping fatal errors (eval {}).

A similar issue will probably arise in Python if a Python binding is
developed that generates a Python wrapper around C++ code. Python
programmers will expect a Python exception for an error like this, not
the termination of the program.

-dave

[Kenton Varda]

Well, note that if you compile with -DNDEBUG, the abort won't happen.  We have a no-exception policy in C++, so I don't think we can replace this with an exception.  Is -DNDEBUG good enough for you or do you want to look for another solution?

[Kenton Varda]

BTW, you can always call IsInitialized() yourself before serializing.  If it returns false, some required fields are missing.

[Dave Bailey]

Thanks Kenton. Yes, I read the Google style guide a while ago and was
thoroughly entertained by the section on (the non-usage of) exceptions
at Google ;-)

Are there any performance implications of compiling generated code
with -DNDEBUG? If not, I think it would be easy for me to add #define
NDEBUG to my generated code before the #include of the protobuf
headers.

Your other suggestion (generating an IsInitialized() call before the
serialization) may work better in that it would allow me to keep the
error message from getting leaked out to stderr. I will look into
that as well. Ideally, I would like the underlying code to:

1) never abort()
2) never print anything to stderr

-dave

On Mar 23, 2:50 pm, Kenton Varda <ken...@google.com> wrote:
> Well, note that if you compile with -DNDEBUG, the abort won't happen.  We
> have a no-exception policy in C++, so I don't think we can replace this with
> an exception.  Is -DNDEBUG good enough for you or do you want to look for
> another solution?
>

[Kenton Varda]

Well, I think you misunderstood a few things about NDEBUG, however it's really not what you want to do anyway so I'm not sure why I suggested it.

What you really want to do is call IsInitialized() yourself before you serialize, and then handle the error however you want.  You can also use FindInitializationErrors() to get a list of missing fields, though you should call IsInitialized() first since it's much faster.

When you actually serialize, use the "Partial" serialization methods to make it skip the IsInitialized() check, e.g. SerializePartialToString().

BTW, you should always compile with -DNDEBUG unless you are building a debug build.  It strips out several debug checks which makes things faster.

[Dave Bailey]

Oh, you're right, sorry. Thanks for the info - it looks like this
should take care of it for me.

-dave

On Mar 23, 4:16 pm, Kenton Varda <ken...@google.com> wrote:
> Well, I think you misunderstood a few things about NDEBUG, however it's
> really not what you want to do anyway so I'm not sure why I suggested it.
>
> What you really want to do is call IsInitialized() yourself before you
> serialize, and then handle the error however you want.  You can also use
> FindInitializationErrors() to get a list of missing fields, though you
> should call IsInitialized() first since it's much faster.
>
> When you actually serialize, use the "Partial" serialization methods to make
> it skip the IsInitialized() check, e.g. SerializePartialToString().
>
> BTW, you should always compile with -DNDEBUG unless you are building a debug
> build.  It strips out several debug checks which makes things faster.
>

[Caleb]

On Mar 23, 7:16 pm, Kenton Varda <ken...@google.com> wrote:

> BTW, you should always compile with -DNDEBUG unless you are building a debug
> build.  It strips out several debug checks which makes things faster.

Note that 'NDEBUG' isnt' a Google-ism, its a Standard C-ism. It
disables the assert() macro, removing all assertions from your code.
This is generally good practice for production code, but just be
careful that you're not doing anything with assert() that has side
effects you depend on, like

assert (++i < 100);

or something silly like that. Never been bit by this myself, but I've
seen it bite others...

[David Anderson]

Going off topic a little, but it is debatable whether assertions
should be disabled in production code. Disabling them doesn't make
incorrect code suddenly become correct. It just disguises the failure
by making the application behave in potentially *very* wonky ways
(which can include everything up to losing/destroying/corrupting
data).

On the other hand, if assertions remain enabled, you will get a clean,
clinical crash when something unexpected happens. Much easier to debug
than a tiny subtle data corruption that happened two weeks ago and
snowballed up to the point where someone noticed.

At Google, 99% of assertions remain enabled at all times. There are
some nuances (as always, mindless dogma gets you nowhere), but in
general, if it should crash during testing, it should also crash in
production. The alternative is entering the forbidden realm of
undefined/unpredictable behavior. We really much prefer to debug a
clean crash with a traceback at the error site, rather than sieving
through weirdness after the fact.

Of course, this policy is supplemented by a heavy focus on automated
testing at all granularities, a focus on designing fault tolerant
software to reduce the impact of a single server crashing, and a lot
of caution (ie. canaries) when upgrading production systems. Such a
policy can't work sanely if your release procedure is "It compiles?
Update the production servers!"

So, the question you need to ask yourself: why is it acceptable, when
something unexpected happens, to silently corrupt data and state in
production and not in testing? In most cases, I find that the answer
is "it is not. Keep the checks."

My 2¢ :-)
- Dave

>
>
> >
>

[Kenton Varda]

On Tue, Mar 24, 2009 at 4:42 PM, David Anderson <da...@natulte.net> wrote:

At Google, 99% of assertions remain enabled at all times. There are

some nuances (as always, mindless dogma gets you nowhere), but in
general, if it should crash during testing, it should also crash in
production. The alternative is entering the forbidden realm of
undefined/unpredictable behavior. We really much prefer to debug a
clean crash with a traceback at the error site, rather than sieving
through weirdness after the fact.

Actually, there are a significant number of Google engineers who fiercely disagree with you.  In many cases it's actually better to log an error and then try your best to do something reasonable than to take down a process which is in the middle of serving dozens of other users. Yes, everything has to be fault-tolerant in the end due to the possibility of machine failures, but assertion failures are much less predictable than machine failures.  It's very easy to accidentally write an assertion that passes testing but then unexpectedly starts triggering in production when something changes.  If your service gets any significant amount of traffic, no reasonable amount of fault-tolerance can protect you against an assertion failure that occurs on even 0.1% of user requests.  Therefore, most critical user-facing servers at Google have rules against assertions that are fatal in production.

Needless to say, there have been endless flamewars about this on internal mailing lists.  I'm not actually sure what side I'm on personally.

In the case of the IsInitialized() check, the main reason that it is debug-only is for speed.  However, the considerations above are also important here.

[David Anderson]

This is true. Since I work mainly in infrastructure that never
directly sees a user, it is cheaper for us to crash immediately,
rather than have yet more error reporting machinery and spending time
to guess adequate answers to insane calls to functions. For us, it is
very cheap to crash, since upstream code will happily compensate, and
the worst case scenario is that we'll delay some batch/soft real time
processing by a couple of seconds. Obviously unacceptable for user
code. I should have better specified the context of my reply.

As you say, there has been endless debate on the subject, and it's
hard to come up with a definite "yes or no" rule. However, I'll note
that what you described for user-facing code, to log an error and try
to continue, is not the behavior you get with -DNDEBUG. By verbosely
logging/reporting an error, using the appropriate tools, you can get
as much information as a crash with traceback, which is identical in
terms of debugability as having crashed and reported the crash site,
except that users don't notice. -DNDEBUG however will just silently
skip all assertions and start stomping around on the stack, leaving
you with no idea of what the hell happened when the corruption does
eventually become critical enough to crash the process.

In other words, 99% of assertions and "non-fatal assertions" remain
enabled in production code. The choice for us is usually between "Do I
crash or make up an answer after verbosely reporting the failure?",
whereas the -DNDEBUG choice is "Do I report and crash, or silently
start unleashing the dragons?". Very different options :-)

- Dave

[Kenton Varda]

On Wed, Mar 25, 2009 at 2:01 AM, David Anderson <da...@natulte.net> wrote:

However, I'll note

that what you described for user-facing code, to log an error and try
to continue, is not the behavior you get with -DNDEBUG.

Depends on the macro.  Our LOG(DFATAL) macro aborts the process in debug mode (when NDEBUG is not defined), but only logs an error in opt mode (when NDEBUG is defined).  That's the behavior I was talking about.

Of course, whether you use LOG(DFATAL) or DCHECK (which dies in debug but does nothing in opt mode) depends on whether you want to pay the price of the check itself in opt mode.