Using an unconditionally-enabled version of assert

[dotnwat]

According to the following comment in the top-level CMake file, Seastar attempts to enable asserts in all modes (e.g. Release). Is it still the case that all asserts in Seastar should be enabled in all modes?

# We want asserts enabled on all modes, but cmake defaults to passing
# -DNDEBUG in some modes. We add -UNDEBUG to our private options to
# reenable it. To force asserts off pass -DNDEBUG in
# Seastar_CXX_FLAGS.

However, Seastar uses assert in both .cc and .hh files. Unfortunately, -DUNDEBUG doesn't apply to header files that are included from other projects. And nor should it: it's reasonable for a project to set -DNDEBUG on a release build because external headers may have performance sensitive asserts that should be compiled out.

In Redpanda we've introduced at least 2 difficult-to-analyze bugs that would have immediately triggered a Seastar assert in a header file, but which was compiled out due to our use of -DNDEBUG.

I'm think about a couple ways to approach this:

1. Move asserts from headers into .cc files
2. Replace assert with something like seastar_assert that is always-on (or configurable).
3. ??

In Redpanda we went with (2). Any suggestions/comments on approaching this?

[travisdowns]

(2) seems like a good idea with little downside? I like it.

It also opens the door to a slightly more featureful assert assert, which I've often missed when writing seastar-side code, e.g., taking some arguments to print.

Worth noting that the current situation is almost certainly an ODR violation when the same function is compiled with and without asserts:

For a given entity, each definition must have the same sequence of tokens.

Though I don't think this form is likely to bite. What is more likely to bite is if any structure changes based on NDEBUG (which is a useful thing to do).

[avikivity]

I like 2 (but SEASTAR_ASSERT so it's clearly not a function).

See scylladb/scylladb@aa1270a

[dotnwat]

🙏

[nyh]

I agree with you and Avi, the standard C assert() has been made almost unusable because of this NDEBUG thing, so it makes sense for each library to have its own version of assert() and its own rules when assertions are disabled and enabled, and possibly how they are printed (e.g., Seastar has its own logging mechanism, maybe assert should use that).

I suggest that in Seastar, we should use the on_internal_error() that we already have. Let me explain why:

It does make sense for assert to behave differently in development and release builds. ‎While in debug builds you might want an immediate crash and core dump in case of bug to make it easier to find and debug - this can lead to disasters in production applications, where a bug in some small request or operation, that should have caused that specific operation to fail, instead causes the entire server to crash on an assertion failure. In the ScyllaDB project we've been bit by this problem a lot, and I opened an issue scylladb/scylladb#7871 to "exorcise" the assert()s from ScyllaDB. One of the alternatives we ended up using a lot is already in Seastar and maybe it should use it internally - it is on_internal_error().

on_internal_error() differs from assert that it crashes the entire application in non-release builds, but in release builds it falls back to throwing an exception. Not to doing nothing (like NDEBUG) but to throwing an exception. By throwing an exception, we avoid continuing to code which we know won't work because its assumptions don't hold. We hope (and usually, this pans out) that the exception will allow us to get out of one problematic request that exposed this bug, and continuing processing different requests that won't expose this bug, those might work.

I wonder if we can convert most assert() in Seastar to using on_internal_error(), like we've been trying to do in ScyllaDB.

[nyh]

I agree with you and Avi, the standard C assert() has been made almost unusable because of this NDEBUG thing, so it makes sense for each library to have its own version of assert() and its own rules when assertions are disabled and enabled, and possibly how they are printed (e.g., Seastar has its own logging mechanism, maybe assert should use that).

Incidentally, the logging issue is important. In ScyllaDB issue scylladb/scylladb#14403 we had a user whose ScyllaDB server crashed for no apparent reason - no error was logged. It turns out the server used assert(), had an assertion failure, but the assertion failure message didn't appear in the log. It should.

[nyh]

By the way, regarding on_internal_error(), one thing that always annoyed me with it is that it needs a logger in the parameter, which makes it messy (and possibly inefficient) to use in header files. In ScyllaDB I simply created a version of on_internal_error() - see https://github.com/scylladb/scylladb/blob/master/utils/on_internal_error.hh and https://github.com/scylladb/scylladb/blob/master/utils/on_internal_error.cc - which uses one fixed logger, and the logging code happens out-of-line. We should probably do this in Seastar too.