STYLE.md
clang-format off must be closed with clang-format on.
To run these checks locally, see Support Tools.doFoo())._ postfix (e.g., int foo_;).RoundRobin).TODO(foobar): blah.using FooPtr = std::unique_ptr<Foo>;using BarSharedPtr = std::shared_ptr<Bar>;using BlahConstSharedPtr = std::shared_ptr<const Blah>;int* foo) should not be type aliased.absl::optional<std::reference_wrapper<T>> has a helper class in envoy/common/optref.h, and is type aliased:
using FooOptRef = OptRef<T>;using FooOptConstRef = OptRef<const T>;&&.
E.g., void onHeaders(Http::HeaderMapPtr&& headers, ...). The rationale for this is that it
forces the caller to specify std::move(...) or pass a temporary and makes the intention at
the callsite clear. Otherwise, it's difficult to tell if a const reference is actually being
passed to the called function. This is true even for std::unique_ptr.unique_ptr over shared_ptr wherever possible. unique_ptr makes ownership in
production code easier to reason about. Note that this creates some test oddities where
production code requires a unique_ptr but the test must still have access to the memory
the production code is using (mock or otherwise). In these cases it is acceptable to allocate
raw memory in a test and return it to the production code with the expectation that the
production code will hold it in a unique_ptr and free it. Envoy uses the factory pattern
quite a bit for these cases. (Search the code for "factory").OptRef<const T> or const Type& as return types for functions
that return const references to existing objects when there is no intention of mutation.
If the caller needs to take ownership of the returned object via a shared_ptr, add a separate
function with a SharedPtr suffix that returns a shared pointer to
extend ownership. For example:
const ConnectionInfoProvider& connectionInfoProvider() const;ConnectionInfoProviderSharedPtr connectionInfoProviderSharedPtr() const;OptRef<const Route> route() const;RouteConstSharedPtr routeSharedPtr() const;std::string. We encourage the use of the
advice in the C++ FAQ on the static initialization
fiasco for
how to best handle this.kConstantVar.
In the Envoy codebase we use ConstantVar or CONSTANT_VAR. If you pick CONSTANT_VAR,
please be certain the name is globally significant to avoid potential conflicts with #defines,
which are not namespace-scoped, and may appear in externally controlled header files.@param to describe
parameters and @return <return-type> for return values. Internal comments for
methods and member variables may be regular C++ // comments or Doxygen at
developer discretion. Where possible, methods should have meaningful
documentation on expected input and state preconditions.#pragma once.main() functions. When code cannot be placed inside the
Envoy namespace there should be a comment of the form // NOLINT(namespace-envoy) at
the top of the file.test directory is intended to be called only
from test code then it should have a name that ends in ForTest() such as aMethodForTest().
In most cases tests can and should be structured so this is not necessary.ABSL_GUARDED_BY, should be used for shared state guarded by
locks/mutexes.A few general notes on our error handling philosophy:
sockaddr after a successful call to accept(), pthread_create(),
pthread_join(), etc. However, system calls that require permissions may cause likely errors in
some deployments and need graceful error handling.RELEASE_ASSERT following a third party library call, or an obvious
crash on a subsequent line via null pointer dereference. This rule is again based on the
philosophy that the engineering costs of properly handling these cases are not worth it. Time is
better spent designing proper system controls that shed load if resource usage becomes too high,
etc.--admin-address-path option). If degraded mode error handling is implemented, we require
that there is complete test coverage for the degraded case. Additionally, the user should be
aware of the degraded state minimally via an error log of level warn or greater and via the
increment of a stat.The following macros are available:
RELEASE_ASSERT: fatal check.ASSERT: fatal check in debug-only builds. These should be used to document (and check in
debug-only builds) program invariants.ENVOY_BUG: logs and increments a stat in release mode, fatal check in debug builds. These
should be used where it may be useful to detect if an efficient condition is violated in
production (and fatal check in debug-only builds). This will also log a stack trace
of the previous calls leading up to ENVOY_BUG.ENVOY_NOTIFICATION: logs and increments the envoy_notifications counter. These should be
used where it may be useful to detect if an efficient condition is met in production, for
example before rolling out a potentially disruptive configuration change.Sub-macros alias the macros above and can be used to annotate specific situations:
ENVOY_BUG_ALPHA (alias ENVOY_BUG): Used for alpha or rapidly changing protocols that need
detectability on probable conditions or invariants.Per above it's acceptable to turn failures into crash semantics via RELEASE_ASSERT(condition) or
PANIC(message) if there is no other sensible behavior, e.g. in OOM (memory/FD) scenarios.
Do not ASSERT on conditions imposed by the external environment. Either add error handling
(potentially with an ENVOY_BUG for detectability) or RELEASE_ASSERT if the condition indicates
that the process is unrecoverable.
Use ASSERT and ENVOY_BUG liberally, but do not use them for things that will crash in an obvious
way in a subsequent line. E.g., do not do ASSERT(foo != nullptr); foo->doSomething();.
Use ASSERTs for true invariants and well-defined conditions that are useful for tests,
debug-only checks and documentation. They may be ENVOY_BUGs if performance allows, see point
below.
ENVOY_BUGs provide detectability and more confidence than an ASSERT. They are useful for
non-trivial conditions, those with complex control flow, and rapidly changing protocols. Testing
should be added to ensure that Envoy can continue to operate even if an ENVOY_BUG condition is
violated.
Annotate conditions with comments on belief or reasoning, for example Condition is guaranteed by caller foo or Condition is likely to hold after processing through external library foo.
Macro usage should be understandable to a reader. Add comments if not. They should be robust to future changes.
Note that there is a gray line between external environment failures and program invariant
violations. For example, memory corruption due to a security issue (a bug, deliberate buffer
overflow etc.) might manifest as a violation of program invariants or as a detectable condition in
the external environment (e.g. some library returning a highly unexpected error code or buffer
contents). Unfortunately no rule can cleanly cover when to use RELEASE_ASSERT vs. ASSERT. In
general we view ASSERT as the common case and RELEASE_ASSERT as the uncommon case, but
experience and judgment may dictate a particular approach depending on the situation. The risk of
process death from RELEASE_ASSERT should be justified with the severity and possibility of the
condition to avoid unintentional crashes. You may use the following guide:
RELEASE_ASSERT.ENVOY_BUG.ASSERT.Below is a guideline for macro usage. The left side of the table has invariants and the right side
has error conditions that can be triggered and should be gracefully handled. ENVOY_BUG represents
a middle ground that can be used for uncertain conditions that need detectability. ENVOY_BUGs can
also be added for errors if they warrant detection.
ASSERT/RELEASE_ASSERT | ENVOY_BUG | Error handling and Testing |
|---|---|---|
| Low level invariants in data structures | ||
| Simple, provable internal class invariants | Complex, uncertain internal class invariants (e.g. need detectability if violated) | |
| Provable (pre/post)-conditions | Complicated but likely (pre-/post-) conditions that are low-risk (Envoy can continue safely) | Triggerable or uncertain conditions, may be based on untrusted data plane traffic or an extensions’ contract. |
Conditions in alpha or changing extensions that need detectability. (ENVOY_BUG_ALPHA) | ||
| Unlikely environment errors after process initialization that would otherwise crash | Likely environment errors, e.g. return codes from untrusted extensions, dependencies or system calls, network error, bad data read, permission based errors, etc. | |
| Fatal crashing events. e.g. OOMs, deadlocks, no process recovery possible |
Tests should be hermetic, i.e. have all dependencies explicitly captured and not depend on the local environment. In general, there should be no non-local network access. In addition:
Port numbers should not be hardcoded. Tests should bind to port zero and then discover the bound
port when needed. This avoids flakes due to conflicting ports and allows tests to be executed
concurrently by Bazel. See
test/integration/integration_test.h and
test/common/network/listener_impl_test.cc
for examples of tests that do this.
Paths should be constructed using:
TestEnvironment for C++ tests.${TEST_TMPDIR} (for writable temporary space) or ${TEST_SRCDIR} for read-only access to
test inputs in shell tests.{{ test_tmpdir }}, {{ test_rundir }} and {{ test_udsdir }} respectively for JSON templates.
{{ test_udsdir }} is provided for pathname based Unix Domain Sockets, which must fit within a
108 character limit on Linux, a property that might not hold for {{ test_tmpdir }}.Tests should be deterministic. They should not rely on randomness or details
such as the current time. Instead, mocks such as
MockRandomGenerator and
SimulatedTimeSystem should be used.