Skip to content

logging: optional details for ASSERT #3934

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
htuch merged 2 commits into from
Aug 3, 2018
Merged

logging: optional details for ASSERT #3934

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14cf57b
Adding optional logs for asserts
alyssawilk Jul 25, 2018
49f9a9e
updated comment
alyssawilk Aug 2, 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
12 changes: 10 additions & 2 deletions source/common/common/assert.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,19 @@ namespace Envoy {
} while (false)

#ifndef NDEBUG
#define ASSERT(X) RELEASE_ASSERT(X, "")
#define _ASSERT_ORIGINAL(X) RELEASE_ASSERT(X, "")
#define _ASSERT_VERBOSE(X, Y) RELEASE_ASSERT(X, Y)
#define _ASSERT_SELECTOR(_1, _2, ASSERT_MACRO, ...) ASSERT_MACRO

// If ASSERT is called with one argument, the ASSERT_SELECTOR will return
// _ASSERT_ORIGINAL and this will call _ASSERT_ORIGINAL(__VA_ARGS__).
// If ASSERT is called with two arguments, ASSERT_SELECTOR will return
// _ASSERT_VERBOSE, and this will call _ASSERT_VERBOSE,(__VA_ARGS__)
#define ASSERT(...) _ASSERT_SELECTOR(__VA_ARGS__, _ASSERT_VERBOSE, _ASSERT_ORIGINAL)(__VA_ARGS__)
#else
// This non-implementation ensures that its argument is a valid expression that can be statically
// casted to a bool, but the expression is never evaluated and will be compiled away.
#define ASSERT(X) \
#define ASSERT(X, ...) \
do { \
constexpr bool __assert_dummy_variable = false && static_cast<bool>(X); \
(void)__assert_dummy_variable; \
Expand Down
2 changes: 1 addition & 1 deletion source/common/network/connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ void ConnectionImpl::onWriteReady() {
}

void ConnectionImpl::setConnectionStats(const ConnectionStats& stats) {
ASSERT(!connection_stats_);
ASSERT(!connection_stats_, "Perhaps there are duplicate filters in your configuration?");

@mattklein123 mattklein123 Jul 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe this is what @htuch was asking about, but from a quick glance this seems non-obvious to me. How is this assert hit if there are duplicate filters?

@alyssawilk alyssawilk Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you configure 2 http connection manager filters, each one has initialzeReadFitlerCallbacks called on it and calls setConnectionStats. The second setConnectionStats causes an assert fail.

As htuch@ says the real fix is to detect the broken config on start-up but if two filters both setConnectionStats it's nicer IMO to get an explanation than just an assert fail.

connection_stats_.reset(new ConnectionStats(stats));
}

Expand Down
14 changes: 13 additions & 1 deletion test/common/common/assert_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace Envoy {

TEST(Assert, VariousLogs) {
TEST(ReleaseAssert, VariousLogs) {
Logger::StderrSinkDelegate stderr_sink(Logger::Registry::getSink()); // For coverage build.
EXPECT_DEATH({ RELEASE_ASSERT(0, ""); }, ".*assert failure: 0.*");
EXPECT_DEATH({ RELEASE_ASSERT(0, "With some logs"); },
Expand All @@ -13,4 +13,16 @@ TEST(Assert, VariousLogs) {
".*assert failure: 0 == EAGAIN. Details: using fmt.*");
}

TEST(Assert, VariousLogs) {
#ifndef NDEBUG
EXPECT_DEATH({ ASSERT(0); }, ".*assert failure: 0.*");
EXPECT_DEATH({ ASSERT(0, ""); }, ".*assert failure: 0.*");
EXPECT_DEATH({ ASSERT(0, "With some logs"); }, ".*assert failure: 0. Details: With some logs.*");
#else
ASSERT(0);
ASSERT(0, "");
ASSERT(0, "With some logs");
#endif
}

} // namespace Envoy