Skip to content

Add quic_expect_bug_impl.h, (spdy|http2)_logging_impl.h, (spdy|http2)_bug_tracker_impl.h QUICHE platform implementation #6339

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 7 commits into from
Mar 26, 2019
Merged

Add quic_expect_bug_impl.h, (spdy|http2)_logging_impl.h, (spdy|http2)_bug_tracker_impl.h QUICHE platform implementation #6339

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7d3314a
Add quic_expect_bug_impl.h.
wu-bin Mar 21, 2019
113f47c
Add spdy_logging_impl.h and spdy_bug_tracker_impl.h.
wu-bin Mar 21, 2019
9bf4b26
Add http2_logging_impl.h and http2_bug_tracker_impl.h.
wu-bin Mar 21, 2019
c338d2f
s/VLOGs/VLOG/ to make spelling checker happy.
wu-bin Mar 21, 2019
105d171
Update per review comment.
wu-bin Mar 21, 2019
751d882
Merge remote-tracking branch 'upstream/master' into h2_platform_loggi…
wu-bin Mar 22, 2019
f9f7cde
Add a TODO for QUIC_BUG to implement exponential back off.
wu-bin Mar 25, 2019
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
8 changes: 5 additions & 3 deletions bazel/external/quiche.BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,19 @@ cc_library(
name = "http2_platform",
hdrs = [
"quiche/http2/platform/api/http2_arraysize.h",
"quiche/http2/platform/api/http2_bug_tracker.h",

@danzh2010 danzh2010 Mar 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Q please: do these API files need to be wrapped in envoy_select_quiche if their impl's are wrapping with it?

@wu-bin wu-bin Mar 21, 2019

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.

It's not necessary but I moved some to envoy_select_quiche.

IIUC, this (and some other) library is not a "real" library - it does not have a "srcs" section, so "bazel build" this library "succeeds" without doing anything. But

  • if a dependency(like http2_platform_test) uses this library, that dependency will fail to build.
  • the library will fail to build as soon as we add a .cc to the "srcs" section.
    In my past PRs, I add envoy_select_quiche when 1) ci fails on the files I'm adding or 2) I think ci will fail on the files I'm adding. It is really a bandaid to allow us to continue working on platform impls before the root of the build problem is fixed.

"quiche/http2/platform/api/http2_containers.h",
"quiche/http2/platform/api/http2_estimate_memory_usage.h",
"quiche/http2/platform/api/http2_export.h",
"quiche/http2/platform/api/http2_flag_utils.h",
"quiche/http2/platform/api/http2_logging.h",
"quiche/http2/platform/api/http2_macros.h",
"quiche/http2/platform/api/http2_optional.h",
"quiche/http2/platform/api/http2_ptr_util.h",
"quiche/http2/platform/api/http2_string.h",
"quiche/http2/platform/api/http2_string_piece.h",
# TODO: uncomment the following files as implementations are added.
# "quiche/http2/platform/api/http2_bug_tracker.h",
# "quiche/http2/platform/api/http2_flags.h",
# "quiche/http2/platform/api/http2_mock_log.h",
# "quiche/http2/platform/api/http2_reconstruct_object.h",
# "quiche/http2/platform/api/http2_test_helpers.h",
] + envoy_select_quiche(
Expand All @@ -79,10 +79,12 @@ cc_library(
name = "spdy_platform",
hdrs = [
"quiche/spdy/platform/api/spdy_arraysize.h",
"quiche/spdy/platform/api/spdy_bug_tracker.h",
"quiche/spdy/platform/api/spdy_containers.h",
"quiche/spdy/platform/api/spdy_endianness_util.h",
"quiche/spdy/platform/api/spdy_estimate_memory_usage.h",
"quiche/spdy/platform/api/spdy_export.h",
"quiche/spdy/platform/api/spdy_logging.h",
"quiche/spdy/platform/api/spdy_mem_slice.h",
"quiche/spdy/platform/api/spdy_ptr_util.h",
"quiche/spdy/platform/api/spdy_string.h",
Expand Down Expand Up @@ -135,6 +137,7 @@ cc_library(
"quiche/quic/platform/api/quic_containers.h",
"quiche/quic/platform/api/quic_endian.h",
"quiche/quic/platform/api/quic_estimate_memory_usage.h",
"quiche/quic/platform/api/quic_expect_bug.h",
"quiche/quic/platform/api/quic_exported_stats.h",
"quiche/quic/platform/api/quic_fallthrough.h",
"quiche/quic/platform/api/quic_flag_utils.h",
Expand All @@ -157,7 +160,6 @@ cc_library(
"quiche/quic/platform/api/quic_thread.h",
# TODO: uncomment the following files as implementations are added.
# "quiche/quic/platform/api/quic_clock.h",
# "quiche/quic/platform/api/quic_expect_bug.h",
# "quiche/quic/platform/api/quic_file_utils.h",
# "quiche/quic/platform/api/quic_flags.h",
# "quiche/quic/platform/api/quic_fuzzed_data_provider.h",
Expand Down
33 changes: 28 additions & 5 deletions source/extensions/quic_listeners/quiche/platform/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,21 @@ envoy_cc_library(
"http2_ptr_util_impl.h",
"http2_string_impl.h",
"http2_string_piece_impl.h",
] + envoy_select_quiche(["http2_string_utils_impl.h"]),
] + envoy_select_quiche([
"http2_bug_tracker_impl.h",
"http2_logging_impl.h",
"http2_string_utils_impl.h",
]),
external_deps = [
"abseil_base",
"abseil_optional",
"abseil_str_format",
],
visibility = ["//visibility:public"],
deps = envoy_select_quiche([":string_utils_lib"]),
deps = envoy_select_quiche([
":quic_platform_logging_impl_lib",
":string_utils_lib",
]),
)

envoy_cc_library(
Expand All @@ -58,9 +65,19 @@ envoy_cc_library(
visibility = ["//visibility:public"],
)

envoy_cc_library(
name = "quic_platform_logging_impl_lib",
srcs = ["quic_logging_impl.cc"],

@danzh2010 danzh2010 Mar 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this need to be wrapped in envoy_select_quiche?

@wu-bin wu-bin Mar 21, 2019

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.

It's not necessary, libraries depend on this is using envoy_select_quiche.

hdrs = [
"quic_bug_tracker_impl.h",
"quic_logging_impl.h",
],
visibility = ["//visibility:public"],
deps = ["//source/common/common:assert_lib"],
)

envoy_cc_library(
name = "quic_platform_base_impl_lib",
srcs = envoy_select_quiche(["quic_logging_impl.cc"]),
hdrs = [
"quic_aligned_impl.h",
"quic_arraysize_impl.h",
Expand All @@ -80,6 +97,7 @@ envoy_cc_library(
"quic_string_piece_impl.h",
"quic_uint128_impl.h",
] + envoy_select_quiche([
"quic_expect_bug_impl.h",
"quic_logging_impl.h",
"quic_mock_log_impl.h",
"quic_stack_trace_impl.h",
Expand All @@ -97,6 +115,7 @@ envoy_cc_library(
],
visibility = ["//visibility:public"],
deps = ["@com_googlesource_quiche//:quic_platform_export"] + envoy_select_quiche([
":quic_platform_logging_impl_lib",
"//include/envoy/thread:thread_interface",
"//source/common/common:assert_lib",
"//source/server:backtrace_lib",
Expand All @@ -114,7 +133,6 @@ envoy_cc_library(
"quic_mutex_impl.h",
"quic_str_cat_impl.h",
] + envoy_select_quiche([
"quic_bug_tracker_impl.h",
"quic_hostname_utils_impl.h",
"quic_string_utils_impl.h",
"quic_test_output_impl.h",
Expand Down Expand Up @@ -158,7 +176,11 @@ envoy_cc_library(
"spdy_test_helpers_impl.h",
"spdy_test_utils_prod_impl.h",
"spdy_unsafe_arena_impl.h",
] + envoy_select_quiche(["spdy_string_utils_impl.h"]),
] + envoy_select_quiche([
"spdy_bug_tracker_impl.h",
"spdy_logging_impl.h",
"spdy_string_utils_impl.h",
]),
external_deps = [
"abseil_base",
"abseil_hash",
Expand All @@ -168,6 +190,7 @@ envoy_cc_library(
],
visibility = ["//visibility:public"],
deps = envoy_select_quiche([
":quic_platform_logging_impl_lib",
":string_utils_lib",
"//source/common/common:assert_lib",
]),
Expand Down
13 changes: 13 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/http2_bug_tracker_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#pragma once

// NOLINT(namespace-envoy)
//
// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_bug_tracker_impl.h"

#define HTTP2_BUG_IMPL QUIC_BUG_IMPL
#define HTTP2_BUG_IF_IMPL QUIC_BUG_IF_IMPL
#define FLAGS_http2_always_log_bugs_for_tests_IMPL true
23 changes: 23 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/http2_logging_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#pragma once

// NOLINT(namespace-envoy)
//
// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_logging_impl.h"

#define HTTP2_LOG_IMPL(severity) QUIC_LOG_IMPL(severity)

#define HTTP2_VLOG_IMPL(verbose_level) QUIC_VLOG_IMPL(verbose_level)

#define HTTP2_DLOG_IMPL(severity) QUIC_DLOG_IMPL(severity)

#define HTTP2_DLOG_IF_IMPL(severity, condition) QUIC_DLOG_IF_IMPL(severity, condition)

#define HTTP2_DVLOG_IMPL(verbose_level) QUIC_DVLOG_IMPL(verbose_level)

#define HTTP2_DVLOG_IF_IMPL(verbose_level, condition) QUIC_DVLOG_IF_IMPL(verbose_level, condition)

#define HTTP2_DLOG_EVERY_N_IMPL(severity, n) QUIC_DLOG_EVERY_N_IMPL(severity, n)
10 changes: 5 additions & 5 deletions source/extensions/quic_listeners/quiche/platform/quic_bug_tracker_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "quiche/quic/platform/api/quic_logging.h"
#include "extensions/quic_listeners/quiche/platform/quic_logging_impl.h"

#define QUIC_BUG_IMPL QUIC_LOG(DFATAL)
#define QUIC_BUG_IF_IMPL(condition) QUIC_LOG_IF(DFATAL, condition)
#define QUIC_PEER_BUG_IMPL QUIC_LOG(ERROR)
#define QUIC_PEER_BUG_IF_IMPL(condition) QUIC_LOG_IF(ERROR, condition)
#define QUIC_BUG_IMPL QUIC_LOG_IMPL(DFATAL)

@alyssawilk alyssawilk Mar 25, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

preexisted, but doesn't QUIC_BUG work based on the exponential back-off, and could cause serious perf problems if we don't do that?
Should we get a TODO to get around to that work?

@wu-bin wu-bin Mar 25, 2019

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.

QUIC_BUG has the exponential back off in google3 but not Chromium. It, like other error logs, could cause serious perf problems if it happens too frequently.

TODO added to implement exponential back off in envoy.

@alyssawilk alyssawilk Mar 25, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah, per-packet logging server side scales less well than client side :-) thanks for the TODO

#define QUIC_BUG_IF_IMPL(condition) QUIC_LOG_IF_IMPL(DFATAL, condition)
#define QUIC_PEER_BUG_IMPL QUIC_LOG_IMPL(ERROR)
#define QUIC_PEER_BUG_IF_IMPL(condition) QUIC_LOG_IF_IMPL(ERROR, condition)
16 changes: 16 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/quic_expect_bug_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#pragma once

// NOLINT(namespace-envoy)
//
// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_logging_impl.h"
#include "extensions/quic_listeners/quiche/platform/quic_mock_log_impl.h"

#define EXPECT_QUIC_BUG_IMPL(statement, regex) \
EXPECT_QUIC_DFATAL_IMPL(statement, testing::ContainsRegex(regex))

#define EXPECT_QUIC_PEER_BUG_IMPL(statement, regex) \
EXPECT_QUIC_LOG_IMPL(statement, ERROR, testing::ContainsRegex(regex))
17 changes: 16 additions & 1 deletion source/extensions/quic_listeners/quiche/platform/quic_logging_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace quic {

namespace {
std::atomic<int> g_verbosity_threshold;
std::atomic<bool> g_dfatal_exit_disabled;

// Pointer to the global log sink, usually it is nullptr.
// If not nullptr, as in some tests, the sink will receive a copy of the log message right after the
Expand Down Expand Up @@ -39,7 +40,15 @@ QuicLogEmitter::~QuicLogEmitter() {
}

if (level_ == FATAL) {
abort();
#ifdef NDEBUG
bool is_dfatal = false;
#else
bool is_dfatal = true;
#endif

if (!(is_dfatal && g_dfatal_exit_disabled)) {
abort();
}

@danzh2010 danzh2010 Mar 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you combine this if block with the #ifdef NDEBUG block above?

@wu-bin wu-bin Mar 21, 2019

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.

Done.

}
}

Expand All @@ -49,6 +58,12 @@ void SetVerbosityLogThreshold(int new_verbosity) {
g_verbosity_threshold.store(new_verbosity, std::memory_order_relaxed);
}

bool IsDFatalExitDisabled() { return g_dfatal_exit_disabled.load(std::memory_order_relaxed); }
Comment thread
htuch marked this conversation as resolved.

void SetDFatalExitDisabled(bool is_disabled) {
g_dfatal_exit_disabled.store(is_disabled, std::memory_order_relaxed);
}

QuicLogSink* SetLogSink(QuicLogSink* new_sink) {
absl::MutexLock lock(&g_quic_log_sink_mutex);
QuicLogSink* old_sink = g_quic_log_sink.load(std::memory_order_relaxed);
Expand Down
8 changes: 8 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/quic_logging_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
// TODO(wub): Implement QUIC_LOG_FIRST_N_IMPL.
#define QUIC_LOG_FIRST_N_IMPL(severity, n) QUIC_LOG_IMPL(severity)

// TODO(wub): Implement QUIC_LOG_EVERY_N_IMPL.
#define QUIC_LOG_EVERY_N_IMPL(severity, n) QUIC_LOG_IMPL(severity)

// TODO(wub): Implement QUIC_LOG_EVERY_N_SEC_IMPL.
#define QUIC_LOG_EVERY_N_SEC_IMPL(severity, seconds) QUIC_LOG_IMPL(severity)

Expand All @@ -64,6 +67,7 @@
#define QUIC_DLOG_IMPL(severity) QUIC_COMPILED_OUT_LOG()
#define QUIC_DLOG_IF_IMPL(severity, condition) QUIC_COMPILED_OUT_LOG()
#define QUIC_DLOG_INFO_IS_ON_IMPL() 0
#define QUIC_DLOG_EVERY_N_IMPL(severity, n) QUIC_COMPILED_OUT_LOG()
#define QUIC_NOTREACHED_IMPL()
#else
// Debug build
Expand All @@ -72,6 +76,7 @@
#define QUIC_DLOG_IMPL(severity) QUIC_LOG_IMPL(severity)
#define QUIC_DLOG_IF_IMPL(severity, condition) QUIC_LOG_IF_IMPL(severity, condition)
#define QUIC_DLOG_INFO_IS_ON_IMPL() QUIC_LOG_INFO_IS_ON_IMPL()
#define QUIC_DLOG_EVERY_N_IMPL(severity, n) QUIC_LOG_EVERY_N_IMPL(severity, n)
#define QUIC_NOTREACHED_IMPL() NOT_REACHED_GCOVR_EXCL_LINE
#endif

Expand Down Expand Up @@ -133,6 +138,9 @@ inline bool IsVerboseLogEnabled(int verbosity) {
return IsLogLevelEnabled(INFO) && verbosity <= GetVerbosityLogThreshold();
}

bool IsDFatalExitDisabled();
void SetDFatalExitDisabled(bool is_disabled);

// QuicLogSink is used to capture logs emitted from the QUIC_LOG... macros.
class QuicLogSink {
public:
Expand Down
40 changes: 40 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/quic_mock_log_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,24 @@ class QuicEnvoyMockLog : public QuicLogSink {
bool is_capturing_;
};

// ScopedDisableExitOnDFatal is used to disable exiting the program when we encounter a
// QUIC_LOG(DFATAL) within the current block. After we leave the current block, the previous
// behavior is restored.
class ScopedDisableExitOnDFatal {
public:
ScopedDisableExitOnDFatal() : previous_value_(IsDFatalExitDisabled()) {
SetDFatalExitDisabled(true);
}

ScopedDisableExitOnDFatal(const ScopedDisableExitOnDFatal&) = delete;
ScopedDisableExitOnDFatal& operator=(const ScopedDisableExitOnDFatal&) = delete;

~ScopedDisableExitOnDFatal() { SetDFatalExitDisabled(previous_value_); }

private:
const bool previous_value_;
};

} // namespace quic

using QuicMockLogImpl = quic::QuicEnvoyMockLog;
Expand All @@ -57,3 +75,25 @@ using QuicMockLogImpl = quic::QuicEnvoyMockLog;

#define EXPECT_QUIC_LOG_CALL_CONTAINS_IMPL(log, level, content) \
EXPECT_CALL(log, Log(quic::level, testing::HasSubstr(content)))

// Not part of the api exposed by quic_mock_log.h. This is used by
// quic_expect_bug_impl.h.
#define EXPECT_QUIC_LOG_IMPL(statement, level, matcher) \
do { \
quic::QuicEnvoyMockLog mock_log; \
EXPECT_CALL(mock_log, Log(quic::level, matcher)).Times(testing::AtLeast(1)); \
mock_log.StartCapturingLogs(); \
{ statement; } \
mock_log.StopCapturingLogs(); \
if (!testing::Mock::VerifyAndClear(&mock_log)) { \
GTEST_NONFATAL_FAILURE_(""); \
} \
} while (false)

#define EXPECT_QUIC_DFATAL_IMPL(statement, matcher) \
EXPECT_QUIC_LOG_IMPL( \
{ \
quic::ScopedDisableExitOnDFatal disable_exit_on_dfatal; \
statement; \
}, \
DFATAL, matcher)
13 changes: 13 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/spdy_bug_tracker_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#pragma once

// NOLINT(namespace-envoy)
//
// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_bug_tracker_impl.h"

#define SPDY_BUG_IMPL QUIC_BUG_IMPL
#define SPDY_BUG_IF_IMPL QUIC_BUG_IF_IMPL
#define FLAGS_spdy_always_log_bugs_for_tests_impl true
21 changes: 21 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/spdy_logging_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#pragma once

// NOLINT(namespace-envoy)
//
// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_logging_impl.h"

#define SPDY_LOG_IMPL(severity) QUIC_LOG_IMPL(severity)

#define SPDY_VLOG_IMPL(verbose_level) QUIC_VLOG_IMPL(verbose_level)

#define SPDY_DLOG_IMPL(severity) QUIC_DLOG_IMPL(severity)

#define SPDY_DLOG_IF_IMPL(severity, condition) QUIC_DLOG_IF_IMPL(severity, condition)

#define SPDY_DVLOG_IMPL(verbose_level) QUIC_DVLOG_IMPL(verbose_level)

#define SPDY_DVLOG_IF_IMPL(verbose_level, condition) QUIC_DVLOG_IF_IMPL(verbose_level, condition)
6 changes: 4 additions & 2 deletions test/extensions/quic_listeners/quiche/platform/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ envoy_package()

envoy_cc_test(
name = "http2_platform_test",
srcs = ["http2_platform_test.cc"],
srcs = envoy_select_quiche(["http2_platform_test.cc"]),
external_deps = ["quiche_http2_platform"],
deps = [
"//test/test_common:logging_lib",
"//test/test_common:utility_lib",
],
)
Expand All @@ -39,9 +40,10 @@ envoy_cc_test(

envoy_cc_test(
name = "spdy_platform_test",
srcs = ["spdy_platform_test.cc"],
srcs = envoy_select_quiche(["spdy_platform_test.cc"]),
external_deps = ["quiche_spdy_platform"],
deps = [
"//test/test_common:logging_lib",
"//test/test_common:utility_lib",
],
)
Loading