From 5efd11a838ba0224bfa12ab130f62d4a5876d849 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Thu, 26 Mar 2020 17:03:11 -0400 Subject: [PATCH 01/11] Add Status and StatusOr classes. These classes are to be used for removing exceptions in HTTP codecs. Signed-off-by: Yan Avlasov --- bazel/repositories.bzl | 4 + source/common/http/BUILD | 22 +++++ source/common/http/status.cc | 68 +++++++++++++ source/common/http/status.h | 149 +++++++++++++++++++++++++++++ source/common/http/status_or.h | 144 ++++++++++++++++++++++++++++ test/common/http/BUILD | 16 ++++ test/common/http/status_or_test.cc | 135 ++++++++++++++++++++++++++ test/common/http/status_test.cc | 112 ++++++++++++++++++++++ 8 files changed, 650 insertions(+) create mode 100644 source/common/http/status.cc create mode 100644 source/common/http/status.h create mode 100644 source/common/http/status_or.h create mode 100644 test/common/http/status_or_test.cc create mode 100644 test/common/http/status_test.cc diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index d2bbc33309619..31f255cc030a3 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -511,6 +511,10 @@ def _com_google_absl(): name = "abseil_algorithm", actual = "@com_google_absl//absl/algorithm:algorithm", ) + native.bind( + name = "abseil_variant", + actual = "@com_google_absl//absl/types:variant", + ) def _com_google_protobuf(): _repository_impl("rules_python") diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 193decf644392..22b4c6046c730 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -393,3 +393,25 @@ envoy_cc_library( "//source/common/common:logger_lib", ], ) + +envoy_cc_library( + name = "status_lib", + srcs = ["status.cc"], + hdrs = ["status.h"], + external_deps = [ + "abseil_base", + "abseil_optional", + ], + deps = [ + "//include/envoy/http:codes_interface", + ], +) + +envoy_cc_library( + name = "status_or_lib", + hdrs = ["status_or.h"], + external_deps = [ + "abseil_variant", + ], + deps = [":status_lib"], +) diff --git a/source/common/http/status.cc b/source/common/http/status.cc new file mode 100644 index 0000000000000..b30dac46fc624 --- /dev/null +++ b/source/common/http/status.cc @@ -0,0 +1,68 @@ +#include "common/http/status.h" + +#include "absl/strings/str_cat.h" + +namespace Envoy { +namespace Http { + +namespace { + +std::string StatusCodeToString(StatusCode code) { + switch (code) { + default: + return ""; + case StatusCode::Ok: + return "OK"; + case StatusCode::CodecProtocolError: + return "CodecProtocolError"; + case StatusCode::BufferFloodError: + return "BufferFloodError"; + case StatusCode::PrematureResponse: + return "PrematureResponse"; + case StatusCode::CodecClientError: + return "CodecClientError"; + } +} + +} // namespace + +void Status::Unref() { + // Fast path: if ref==1, there is no need for a RefCountDec (since + // this is the only reference and therefore no other thread is + // allowed to be mucking with r). + if (rep_ && (rep_->ref_.load(std::memory_order_acquire) == 1 || + rep_->ref_.fetch_sub(1, std::memory_order_acq_rel) - 1 == 0)) { + delete rep_; + } +} + +Status::Status(StatusCode code, absl::string_view msg) + : rep_(code != StatusCode::Ok ? new StatusRep(code, msg) : nullptr) { + Ref(); +} + +Status::Status(StatusCode code, Http::Code http_code, absl::string_view msg) + : rep_(code != StatusCode::Ok ? new StatusRep(code, http_code, msg) : nullptr) { + Ref(); +} + +Status::~Status() { Unref(); } + +const std::string* Status::EmptyString() { + static std::string* empty_string = new std::string; + return empty_string; +} + +std::string Status::ToStringError() const { + std::string text; + if (rep_->http_code_.has_value()) { + absl::StrAppend(&text, StatusCodeToString(Code()), ", HTTP code ", rep_->http_code_.value(), + ": ", Message()); + } else { + absl::StrAppend(&text, StatusCodeToString(Code()), ": ", Message()); + } + return text; +} + +} // namespace Http +} // namespace Envoy diff --git a/source/common/http/status.h b/source/common/http/status.h new file mode 100644 index 0000000000000..04e6af7184ace --- /dev/null +++ b/source/common/http/status.h @@ -0,0 +1,149 @@ +#pragma once + +#include +#include + +#include "envoy/http/codes.h" + +#include "absl/base/attributes.h" +#include "absl/strings/string_view.h" +#include "absl/types/optional.h" + +namespace Envoy { +namespace Http { + +/** + * Status codes for representing classes of codec errors. + */ + +enum class StatusCode : int { + Ok = 0, + CodecProtocolError = 1, + BufferFloodError = 2, + PrematureResponse = 3, + CodecClientError = 4 +}; + +/** + * Class for returning error status. + * This class is to replace the use of exceptions in Envoy's codecs. + * Requirements: + * 1. Represent the type system of existing codec exceptions. + * 2. Highly efficient when the status has NO error. + * 3. Register size, to avoid stack allocations. + * 4. Extensible. + * + * Constructing an error status is likely as heavy as throwing an exception due to a heap + * allocation. + */ + +class ABSL_MUST_USE_RESULT Status final { +public: + // Creates an OK status with no message. + Status() {} + + /** + * Create a status with the specified code and + * error message. If `code == StatusCode::Ok`, `msg` is ignored and an + * object identical to an OK status is constructed. + */ + Status(StatusCode code, absl::string_view msg); + + /** + * Create a status with the specified code, HTTP code and + * error message. If `code == StatusCode::Ok`, `msg` and `http_code` are ignored and an + * object identical to an OK status is constructed. + */ + Status(StatusCode code, Http::Code http_code, absl::string_view msg); + + Status(const Status& other) : rep_(other.rep_) { Ref(); } + Status& operator=(const Status& x); + + // The moved-from state is the Ok. + Status(Status&& other) noexcept : rep_(other.rep_) { other.rep_ = nullptr; } + Status& operator=(Status&&); + + ~Status(); + + // Returns true if the Status is OK. + ABSL_MUST_USE_RESULT bool Ok() const { return rep_ == nullptr; } + + // Returns the error code. + StatusCode Code() const { return Ok() ? StatusCode::Ok : rep_->status_code_; } + + /** + * Returns the error message. Note: prefer ToString() for debug logging. + * This message rarely describes the error code. It is not unusual for the + * error message to be the empty string. + */ + absl::string_view Message() const { return Ok() ? *EmptyString() : rep_->message_; } + + // Returns HTTP code if it was provided during Status construction + absl::optional HttpCode() const { + return Ok() ? absl::nullopt : rep_->http_code_; + } + + /** + * Returns a combination of the error code name, HTTP status if it was specified and the message. + * You can expect the code name and the message to be substrings of the + * result. + * WARNING: Do not depend on the exact format of the result of `ToString()` + * which is subject to change. + */ + std::string ToString() const { return Ok() ? "OK" : ToStringError(); } + + /** + * Ignores any errors. This method does nothing except potentially suppress + * complaints from any tools that are checking that errors are not dropped on + * the floor. + */ + void IgnoreError() const {} + +private: + static const std::string* EmptyString(); + + // Returns string for non-ok Status. + std::string ToStringError() const; + + struct StatusRep { + StatusRep(StatusCode code, absl::string_view msg) + : ref_(0), status_code_(code), message_(msg) {} + StatusRep(StatusCode code, Http::Code http_code, absl::string_view msg) + : ref_(0), status_code_(code), message_(msg), http_code_(http_code) {} + + std::atomic ref_; + StatusCode status_code_; + std::string message_; + absl::optional http_code_; + }; + + void Ref() { + if (rep_) + rep_->ref_.fetch_add(1, std::memory_order_relaxed); + } + void Unref(); + + StatusRep* rep_{nullptr}; +}; + +inline Status& Status::operator=(const Status& other) { + if (other.rep_ != rep_) { + Unref(); + rep_ = other.rep_; + Ref(); + } + return *this; +} + +inline Status& Status::operator=(Status&& other) { + Unref(); + rep_ = other.rep_; + Ref(); + return *this; +} + +// Helper to make code returning Ok status more readable. +inline Status OkStatus() { return Status(); } + +} // namespace Http +} // namespace Envoy diff --git a/source/common/http/status_or.h b/source/common/http/status_or.h new file mode 100644 index 0000000000000..83cc8a1886082 --- /dev/null +++ b/source/common/http/status_or.h @@ -0,0 +1,144 @@ +#pragma once + +#include "common/http/status.h" + +#include "absl/types/variant.h" + +namespace Envoy { +namespace Http { + +/** + * Union of the Status and a T object. + * An object of StatusOr has either usable value or an error + * explaining why the value is not present. + * This class is used to facilitate replacement of exceptions with + * a return code in HTTP codecs. For instance method Foo() that throws an exception and returns an + * int + * + * int Foo() { + * ... + * if (error_condition) { + * throw CodecProtocolException("Error"); + * } + * return valid_value; + * } + * + * can be replaced with: + * + * StatusOr Foo() { + * ... + * if (error_condition) { + * return Status(StatusCode::CodecProtocolError, "Error"); + * } + * return valid_value; + * } + */ +template class StatusOr { + using StatusOrValue = absl::variant; + +public: + /** + * Construct a new StatusOr with the given non-ok status. After calling + * this constructor, calls to Value() will RELEASE_ASSERT-fail. + * + * NOTE: Not explicit - we want to use StatusOr as a return + * value, so it is convenient and sensible to be able to do 'return + * Status(...)' when the return type is StatusOr. + * + * REQUIRES: status != StatusCode::Ok(). This requirement is checked with RELEASE_ASSERT. + */ + StatusOr(const Status& status) : status_or_value_(status) { + RELEASE_ASSERT(!status.Ok(), "Status must not be Ok"); + } + + /** + * Construct a new StatusOr with the given value. + * After calling this constructor, calls to + * Value() will succeed, and calls to Status() will return OK. + * + * NOTE: Not explicit - we want to use StatusOr as a return type + * so it is convenient and sensible to be able to do 'return T()' + * when when the return type is StatusOr. + */ + StatusOr(const T& value) : status_or_value_(value) {} + StatusOr(T&& value) : status_or_value_(std::move(value)) {} + + /** + * Conversion copy constructor and assignment operator, T must be copy constructible from U + * TODO(yanavlasov): disable this constructor for cases where T can be copy constructed from + * StatusOr to avoid ambiguity. + */ + template + StatusOr(const StatusOr& other) + : status_or_value_(other.Ok() ? StatusOrValue(other.Value()) + : StatusOrValue(other.Status())) {} + template StatusOr& operator=(const StatusOr& other) { + status_or_value_ = other.Ok() ? StatusOrValue(other.Value()) : StatusOrValue(other.Status()); + return *this; + } + + /** + * Move constructor is a bit weird. Move operation must leave the moved out of object in a valid + * state. However moved out of Status is in the Ok() state, which makes the StatusOr to be in an + * invalid state, there status is Ok but there is no value. As such move operations move the value + * BUT copy the status, such that the moved out of StatusOr remains valid when it carries Status. + * It is a bit inefficient for error cases, but still allows T to be a move only type + */ + StatusOr(StatusOr&& other) + : status_or_value_(other.Ok() ? StatusOrValue(std::move(other).Value()) + : StatusOrValue(other.Status())) {} + StatusOr& operator=(StatusOr&& other) { + status_or_value_ = + other.Ok() ? StatusOrValue(std::move(other).Value()) : StatusOrValue(other.Status()); + return *this; + } + StatusOr(const StatusOr&) = default; + StatusOr& operator=(const StatusOr&) = default; + + template + StatusOr(StatusOr&& other) + : status_or_value_(other.Ok() ? StatusOrValue(std::move(other).Value()) + : StatusOrValue(other.Status())) {} + template StatusOr& operator=(StatusOr&& other) { + status_or_value_ = + other.Ok() ? StatusOrValue(std::move(other).Value()) : StatusOrValue(other.Status()); + return *this; + } + + /** + * Returns a reference to our status. If this contains a T, then + * returns Status::OK. + */ + const Status& Status() const { + static const class Status ok_status; + return Ok() ? ok_status : absl::get(status_or_value_); + } + + bool Ok() const { return absl::holds_alternative(status_or_value_); } + + // Returns various kinds of references to the current value, or RELEASE_ASSERT if !this->ok(). + const T& Value() const& { + RELEASE_ASSERT(Ok(), "Status must be Ok"); + return absl::get(status_or_value_); + } + T& Value() & { + RELEASE_ASSERT(Ok(), "Status must be Ok"); + return absl::get(status_or_value_); + } + + // Support move operation on underlying value. Invoke using `std::move(status_or).Value()` + T&& Value() && { + RELEASE_ASSERT(Ok(), "Status must be Ok"); + return absl::get(std::move(status_or_value_)); + } + const T&& Value() const&& { + RELEASE_ASSERT(Ok(), "Status must be Ok"); + return absl::get(std::move(status_or_value_)); + } + +private: + StatusOrValue status_or_value_; +}; + +} // namespace Http +} // namespace Envoy diff --git a/test/common/http/BUILD b/test/common/http/BUILD index c7dadc09bc493..55429af6783d2 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -368,3 +368,19 @@ envoy_cc_test( "//source/common/http:path_utility_lib", ], ) + +envoy_cc_test( + name = "status_test", + srcs = ["status_test.cc"], + deps = [ + "//source/common/http:status_lib", + ], +) + +envoy_cc_test( + name = "status_or_test", + srcs = ["status_or_test.cc"], + deps = [ + "//source/common/http:status_or_lib", + ], +) diff --git a/test/common/http/status_or_test.cc b/test/common/http/status_or_test.cc new file mode 100644 index 0000000000000..010107ec81190 --- /dev/null +++ b/test/common/http/status_or_test.cc @@ -0,0 +1,135 @@ +#include + +#include "common/http/status_or.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Http { + +TEST(StatusOr, Status) { + StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); + EXPECT_FALSE(status_or.Ok()); + EXPECT_FALSE(status_or.Status().Ok()); + EXPECT_EQ(StatusCode::CodecProtocolError, status_or.Status().Code()); + EXPECT_EQ("foobar", status_or.Status().Message()); +} + +TEST(StatusOr, CopyStatus) { + StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); + StatusOr status_or_copy(status_or); + EXPECT_FALSE(status_or_copy.Ok()); + EXPECT_FALSE(status_or_copy.Status().Ok()); + EXPECT_EQ(StatusCode::CodecProtocolError, status_or_copy.Status().Code()); + EXPECT_EQ("foobar", status_or_copy.Status().Message()); +} + +TEST(StatusOr, CopyErrorIntoValue) { + StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); + StatusOr status_or_copy(1234); + status_or_copy = status_or; + EXPECT_FALSE(status_or_copy.Ok()); + EXPECT_FALSE(status_or_copy.Status().Ok()); + EXPECT_EQ(StatusCode::CodecProtocolError, status_or_copy.Status().Code()); + EXPECT_EQ("foobar", status_or_copy.Status().Message()); +} + +TEST(StatusOr, MoveStatus) { + StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); + StatusOr status_or_copy(std::move(status_or)); + + // status_or should retain the error status + EXPECT_FALSE(status_or.Ok()); + EXPECT_FALSE(status_or.Status().Ok()); + EXPECT_EQ(StatusCode::CodecProtocolError, status_or.Status().Code()); + EXPECT_EQ("foobar", status_or.Status().Message()); + + EXPECT_FALSE(status_or_copy.Ok()); + EXPECT_FALSE(status_or_copy.Status().Ok()); + EXPECT_EQ(StatusCode::CodecProtocolError, status_or_copy.Status().Code()); + EXPECT_EQ("foobar", status_or_copy.Status().Message()); +} + +TEST(StatusOr, Value) { + StatusOr status_or = 55; + EXPECT_TRUE(status_or.Ok()); + EXPECT_TRUE(status_or.Status().Ok()); + EXPECT_EQ(StatusCode::Ok, status_or.Status().Code()); + EXPECT_EQ("", status_or.Status().Message()); + EXPECT_EQ(55, status_or.Value()); +} + +TEST(StatusOr, CopyValue) { + StatusOr status_or = 55; + StatusOr status_or_copy = status_or; + EXPECT_TRUE(status_or_copy.Ok()); + EXPECT_TRUE(status_or_copy.Status().Ok()); + EXPECT_EQ(StatusCode::Ok, status_or_copy.Status().Code()); + EXPECT_EQ("", status_or_copy.Status().Message()); + EXPECT_EQ(55, status_or_copy.Value()); +} + +TEST(StatusOr, CopyValueIntoError) { + StatusOr status_or = 55; + StatusOr status_or_copy(Status(StatusCode::CodecProtocolError, "foobar")); + status_or_copy = status_or; + EXPECT_TRUE(status_or_copy.Ok()); + EXPECT_TRUE(status_or_copy.Status().Ok()); + EXPECT_EQ(StatusCode::Ok, status_or_copy.Status().Code()); + EXPECT_EQ("", status_or_copy.Status().Message()); + EXPECT_EQ(55, status_or_copy.Value()); +} + +// Validate that StatusOr works with move only types +TEST(StatusOr, MoveOnlyValue) { + StatusOr> status_or(std::make_unique(55)); + EXPECT_TRUE(status_or.Ok()); + EXPECT_TRUE(status_or.Status().Ok()); + EXPECT_EQ(StatusCode::Ok, status_or.Status().Code()); + EXPECT_EQ("", status_or.Status().Message()); + EXPECT_EQ(55, *status_or.Value()); + + StatusOr> another_status_or(std::move(status_or)); + EXPECT_EQ(55, *another_status_or.Value()); + + std::unique_ptr value(std::move(another_status_or).Value()); + EXPECT_EQ(55, *value); + EXPECT_EQ(nullptr, another_status_or.Value()); + EXPECT_TRUE(another_status_or.Ok()); +} + +TEST(StatusOr, CopyConversion) { + struct A { + A(int m) : m_(m) {} + int m_{}; + }; + struct B { + B(const A& a) : n_(10 + a.m_) {} + int n_{}; + }; + + StatusOr a = A(12); + StatusOr b(a); + EXPECT_EQ(22, b.Value().n_); +} + +TEST(StatusOr, MoveConversion) { + struct A { + A(int m) : m_(m) {} + virtual ~A() {} + int m_{}; + }; + struct B : public A { + B(int n) : A(10 + n), n_(n) {} + int n_{}; + }; + + StatusOr> b(std::make_unique(121)); + StatusOr> a(std::move(b)); + EXPECT_EQ(131, a.Value()->m_); + EXPECT_EQ(nullptr, b.Value()); +} + +} // namespace Http +} // namespace Envoy diff --git a/test/common/http/status_test.cc b/test/common/http/status_test.cc new file mode 100644 index 0000000000000..36d552598b6c3 --- /dev/null +++ b/test/common/http/status_test.cc @@ -0,0 +1,112 @@ +#include "common/http/status.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Http { + +TEST(Status, Defaults) { + Status status; + EXPECT_EQ(sizeof(void*), sizeof(status)); + EXPECT_TRUE(status.Ok()); + EXPECT_EQ(StatusCode::Ok, status.Code()); + EXPECT_FALSE(status.HttpCode().has_value()); + EXPECT_TRUE(status.Message().empty()); + EXPECT_EQ("OK", status.ToString()); +} + +TEST(Status, OkStatusIgnoresOtherConstructorParameters) { + Status status(StatusCode::Ok, "foobar"); + EXPECT_TRUE(status.Ok()); + EXPECT_EQ(StatusCode::Ok, status.Code()); + EXPECT_FALSE(status.HttpCode().has_value()); + EXPECT_TRUE(status.Message().empty()); + EXPECT_EQ("OK", status.ToString()); + + Status another_status(StatusCode::Ok, Http::Code::InternalServerError, "foobar"); + EXPECT_TRUE(another_status.Ok()); + EXPECT_EQ(StatusCode::Ok, another_status.Code()); + EXPECT_FALSE(another_status.HttpCode().has_value()); + EXPECT_TRUE(another_status.Message().empty()); + EXPECT_EQ("OK", another_status.ToString()); +} + +TEST(Status, ErrorAndMessage) { + Status status(StatusCode::CodecProtocolError, "foobar"); + EXPECT_FALSE(status.Ok()); + EXPECT_EQ(StatusCode::CodecProtocolError, status.Code()); + EXPECT_FALSE(status.HttpCode().has_value()); + EXPECT_EQ("foobar", status.Message()); + EXPECT_EQ("CodecProtocolError: foobar", status.ToString()); +} + +TEST(Status, ErrorHttpCodeAndMessage) { + Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); + EXPECT_FALSE(status.Ok()); + EXPECT_EQ(StatusCode::BufferFloodError, status.Code()); + EXPECT_TRUE(status.HttpCode().has_value()); + EXPECT_EQ(Http::Code::BadRequest, status.HttpCode().value()); + EXPECT_EQ("foobar", status.Message()); + EXPECT_EQ("BufferFloodError, HTTP code 400: foobar", status.ToString()); +} + +TEST(Status, MoveOkIntoOk) { + Status status; + Status another_status(std::move(status)); + EXPECT_TRUE(status.Ok()); + EXPECT_TRUE(another_status.Ok()); +} + +TEST(Status, MoveOkIntoError) { + Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); + Status another_status; + status = std::move(another_status); + EXPECT_TRUE(status.Ok()); + EXPECT_TRUE(another_status.Ok()); +} + +TEST(Status, MoveErrorIntoOk) { + Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); + Status another_status(std::move(status)); + EXPECT_TRUE(status.Ok()); + EXPECT_FALSE(another_status.Ok()); + EXPECT_EQ(StatusCode::BufferFloodError, another_status.Code()); + EXPECT_TRUE(another_status.HttpCode().has_value()); + EXPECT_EQ(Http::Code::BadRequest, another_status.HttpCode().value()); + EXPECT_EQ("foobar", another_status.Message()); +} + +TEST(Status, CopyOkIntoOk) { + Status status; + Status another_status(status); + EXPECT_TRUE(status.Ok()); + EXPECT_TRUE(another_status.Ok()); +} + +TEST(Status, CopyOkIntoError) { + Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); + Status another_status; + status = another_status; + EXPECT_TRUE(status.Ok()); + EXPECT_TRUE(another_status.Ok()); +} + +TEST(Status, CopyErrorIntoOk) { + Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); + Status another_status(status); + EXPECT_FALSE(status.Ok()); + EXPECT_EQ(StatusCode::BufferFloodError, status.Code()); + EXPECT_TRUE(status.HttpCode().has_value()); + EXPECT_EQ(Http::Code::BadRequest, status.HttpCode().value()); + EXPECT_EQ("foobar", status.Message()); + + EXPECT_FALSE(another_status.Ok()); + EXPECT_EQ(StatusCode::BufferFloodError, another_status.Code()); + EXPECT_TRUE(another_status.HttpCode().has_value()); + EXPECT_EQ(Http::Code::BadRequest, another_status.HttpCode().value()); + EXPECT_EQ("foobar", another_status.Message()); +} + +} // namespace Http +} // namespace Envoy From 9bd9d7f8f976863746273f1876890286b5cf62ee Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 27 Mar 2020 13:34:40 -0400 Subject: [PATCH 02/11] Mollify clang-tidy Signed-off-by: Yan Avlasov --- source/common/http/status.cc | 2 +- source/common/http/status.h | 13 ++++++------- source/common/http/status_or.h | 4 ++-- test/common/http/status_or_test.cc | 10 +++++----- test/common/http/status_test.cc | 10 +++++----- 5 files changed, 19 insertions(+), 20 deletions(-) diff --git a/source/common/http/status.cc b/source/common/http/status.cc index b30dac46fc624..e424596dc94c7 100644 --- a/source/common/http/status.cc +++ b/source/common/http/status.cc @@ -49,7 +49,7 @@ Status::Status(StatusCode code, Http::Code http_code, absl::string_view msg) Status::~Status() { Unref(); } const std::string* Status::EmptyString() { - static std::string* empty_string = new std::string; + static auto* empty_string = new std::string; return empty_string; } diff --git a/source/common/http/status.h b/source/common/http/status.h index 04e6af7184ace..376f3887ae841 100644 --- a/source/common/http/status.h +++ b/source/common/http/status.h @@ -15,7 +15,6 @@ namespace Http { /** * Status codes for representing classes of codec errors. */ - enum class StatusCode : int { Ok = 0, CodecProtocolError = 1, @@ -36,11 +35,10 @@ enum class StatusCode : int { * Constructing an error status is likely as heavy as throwing an exception due to a heap * allocation. */ - class ABSL_MUST_USE_RESULT Status final { public: // Creates an OK status with no message. - Status() {} + Status() = default; /** * Create a status with the specified code and @@ -61,7 +59,7 @@ class ABSL_MUST_USE_RESULT Status final { // The moved-from state is the Ok. Status(Status&& other) noexcept : rep_(other.rep_) { other.rep_ = nullptr; } - Status& operator=(Status&&); + Status& operator=(Status&&) noexcept; ~Status(); @@ -118,15 +116,16 @@ class ABSL_MUST_USE_RESULT Status final { }; void Ref() { - if (rep_) + if (rep_) { rep_->ref_.fetch_add(1, std::memory_order_relaxed); + } } void Unref(); StatusRep* rep_{nullptr}; }; -inline Status& Status::operator=(const Status& other) { +inline Status& Status::operator=(const Status& other) { // NOLINT self assignment if (other.rep_ != rep_) { Unref(); rep_ = other.rep_; @@ -135,7 +134,7 @@ inline Status& Status::operator=(const Status& other) { return *this; } -inline Status& Status::operator=(Status&& other) { +inline Status& Status::operator=(Status&& other) noexcept { Unref(); rep_ = other.rep_; Ref(); diff --git a/source/common/http/status_or.h b/source/common/http/status_or.h index 83cc8a1886082..cf9d4cf163f08 100644 --- a/source/common/http/status_or.h +++ b/source/common/http/status_or.h @@ -84,10 +84,10 @@ template class StatusOr { * BUT copy the status, such that the moved out of StatusOr remains valid when it carries Status. * It is a bit inefficient for error cases, but still allows T to be a move only type */ - StatusOr(StatusOr&& other) + StatusOr(StatusOr&& other) noexcept : status_or_value_(other.Ok() ? StatusOrValue(std::move(other).Value()) : StatusOrValue(other.Status())) {} - StatusOr& operator=(StatusOr&& other) { + StatusOr& operator=(StatusOr&& other) noexcept { status_or_value_ = other.Ok() ? StatusOrValue(std::move(other).Value()) : StatusOrValue(other.Status()); return *this; diff --git a/test/common/http/status_or_test.cc b/test/common/http/status_or_test.cc index 010107ec81190..002cb76e0cfcc 100644 --- a/test/common/http/status_or_test.cc +++ b/test/common/http/status_or_test.cc @@ -18,7 +18,7 @@ TEST(StatusOr, Status) { TEST(StatusOr, CopyStatus) { StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); - StatusOr status_or_copy(status_or); + StatusOr status_or_copy(status_or); // NOLINT - unit test EXPECT_FALSE(status_or_copy.Ok()); EXPECT_FALSE(status_or_copy.Status().Ok()); EXPECT_EQ(StatusCode::CodecProtocolError, status_or_copy.Status().Code()); @@ -40,7 +40,7 @@ TEST(StatusOr, MoveStatus) { StatusOr status_or_copy(std::move(status_or)); // status_or should retain the error status - EXPECT_FALSE(status_or.Ok()); + EXPECT_FALSE(status_or.Ok()); // NOLINT - unit test EXPECT_FALSE(status_or.Status().Ok()); EXPECT_EQ(StatusCode::CodecProtocolError, status_or.Status().Code()); EXPECT_EQ("foobar", status_or.Status().Message()); @@ -95,7 +95,7 @@ TEST(StatusOr, MoveOnlyValue) { std::unique_ptr value(std::move(another_status_or).Value()); EXPECT_EQ(55, *value); - EXPECT_EQ(nullptr, another_status_or.Value()); + EXPECT_EQ(nullptr, another_status_or.Value()); // NOLINT - unit test EXPECT_TRUE(another_status_or.Ok()); } @@ -117,7 +117,7 @@ TEST(StatusOr, CopyConversion) { TEST(StatusOr, MoveConversion) { struct A { A(int m) : m_(m) {} - virtual ~A() {} + virtual ~A() = default; int m_{}; }; struct B : public A { @@ -128,7 +128,7 @@ TEST(StatusOr, MoveConversion) { StatusOr> b(std::make_unique(121)); StatusOr> a(std::move(b)); EXPECT_EQ(131, a.Value()->m_); - EXPECT_EQ(nullptr, b.Value()); + EXPECT_EQ(nullptr, b.Value()); // NOLINT - unit test } } // namespace Http diff --git a/test/common/http/status_test.cc b/test/common/http/status_test.cc index 36d552598b6c3..c01bd4ee923c5 100644 --- a/test/common/http/status_test.cc +++ b/test/common/http/status_test.cc @@ -54,7 +54,7 @@ TEST(Status, ErrorHttpCodeAndMessage) { TEST(Status, MoveOkIntoOk) { Status status; Status another_status(std::move(status)); - EXPECT_TRUE(status.Ok()); + EXPECT_TRUE(status.Ok()); // NOLINT - unit test EXPECT_TRUE(another_status.Ok()); } @@ -63,13 +63,13 @@ TEST(Status, MoveOkIntoError) { Status another_status; status = std::move(another_status); EXPECT_TRUE(status.Ok()); - EXPECT_TRUE(another_status.Ok()); + EXPECT_TRUE(another_status.Ok()); // NOLINT - unit test } TEST(Status, MoveErrorIntoOk) { Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); Status another_status(std::move(status)); - EXPECT_TRUE(status.Ok()); + EXPECT_TRUE(status.Ok()); // NOLINT - unit test EXPECT_FALSE(another_status.Ok()); EXPECT_EQ(StatusCode::BufferFloodError, another_status.Code()); EXPECT_TRUE(another_status.HttpCode().has_value()); @@ -79,7 +79,7 @@ TEST(Status, MoveErrorIntoOk) { TEST(Status, CopyOkIntoOk) { Status status; - Status another_status(status); + Status another_status(status); // NOLINT - unit test EXPECT_TRUE(status.Ok()); EXPECT_TRUE(another_status.Ok()); } @@ -94,7 +94,7 @@ TEST(Status, CopyOkIntoError) { TEST(Status, CopyErrorIntoOk) { Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); - Status another_status(status); + Status another_status(status); // NOLINT - unit test EXPECT_FALSE(status.Ok()); EXPECT_EQ(StatusCode::BufferFloodError, status.Code()); EXPECT_TRUE(status.HttpCode().has_value()); From ed3cea4c5409b7e0855cbd4c5d5382a907da527a Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 6 Apr 2020 11:17:49 -0400 Subject: [PATCH 03/11] Address comments Signed-off-by: Yan Avlasov --- bazel/repositories.bzl | 4 + source/common/common/BUILD | 12 +++ source/common/common/status.cc | 126 ++++++++++++++++++++++++ source/common/common/status.h | 94 ++++++++++++++++++ source/common/http/BUILD | 22 ----- source/common/http/status.cc | 68 ------------- source/common/http/status.h | 148 ----------------------------- source/common/http/status_or.h | 144 ---------------------------- test/common/common/BUILD | 8 ++ test/common/common/status_test.cc | 69 ++++++++++++++ test/common/http/BUILD | 16 ---- test/common/http/status_or_test.cc | 135 -------------------------- test/common/http/status_test.cc | 112 ---------------------- 13 files changed, 313 insertions(+), 645 deletions(-) create mode 100644 source/common/common/status.cc create mode 100644 source/common/common/status.h delete mode 100644 source/common/http/status.cc delete mode 100644 source/common/http/status.h delete mode 100644 source/common/http/status_or.h create mode 100644 test/common/common/status_test.cc delete mode 100644 test/common/http/status_or_test.cc delete mode 100644 test/common/http/status_test.cc diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 31f255cc030a3..29aef098f0af5 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -515,6 +515,10 @@ def _com_google_absl(): name = "abseil_variant", actual = "@com_google_absl//absl/types:variant", ) + native.bind( + name = "abseil_status", + actual = "@com_google_absl//absl/status", + ) def _com_google_protobuf(): _repository_impl("rules_python") diff --git a/source/common/common/BUILD b/source/common/common/BUILD index a4dd7f32b608a..41a2f083dd94e 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -419,3 +419,15 @@ envoy_cc_library( "//source/common/common:utility_lib", ], ) + +envoy_cc_library( + name = "status_lib", + srcs = ["status.cc"], + hdrs = ["status.h"], + external_deps = [ + "abseil_status", + ], + deps = [ + "//include/envoy/http:codes_interface", + ], +) diff --git a/source/common/common/status.cc b/source/common/common/status.cc new file mode 100644 index 0000000000000..c6aa06dfe154a --- /dev/null +++ b/source/common/common/status.cc @@ -0,0 +1,126 @@ +#include "common/common/status.h" + +#include "absl/strings/str_cat.h" + +namespace Envoy { + +namespace { + +const char EnvoyPayloadUrl[] = {"Envoy"}; + +absl::string_view StatusCodeToString(StatusCode code) { + switch (code) { + default: + RELEASE_ASSERT(false, "Unknown status code"); + case StatusCode::CodecProtocolError: + return "CodecProtocolError"; + case StatusCode::BufferFloodError: + return "BufferFloodError"; + case StatusCode::PrematureResponseError: + return "PrematureResponseError"; + case StatusCode::CodecClientError: + return "CodecClientError"; + } +} + +absl::StatusCode ToAbslStatusCode(StatusCode code) { + switch (code) { + default: + RELEASE_ASSERT(false, "Unknown status code"); + case StatusCode::Ok: + return absl::StatusCode::kOk; + case StatusCode::CodecProtocolError: + return absl::StatusCode::kFailedPrecondition; + case StatusCode::BufferFloodError: + return absl::StatusCode::kResourceExhausted; + case StatusCode::PrematureResponseError: + return absl::StatusCode::kInternal; + case StatusCode::CodecClientError: + return absl::StatusCode::kUnimplemented; + } +} + +StatusCode ToEnvoyStatusCode(absl::StatusCode code) { + switch (code) { + default: + RELEASE_ASSERT(false, "Unknown status code"); + case absl::StatusCode::kOk: + return StatusCode::Ok; + case absl::StatusCode::kFailedPrecondition: + return StatusCode::CodecProtocolError; + case absl::StatusCode::kResourceExhausted: + return StatusCode::BufferFloodError; + case absl::StatusCode::kInternal: + return StatusCode::PrematureResponseError; + case absl::StatusCode::kUnimplemented: + return StatusCode::CodecClientError; + } +} + +} // namespace + +std::string ToString(const Status& status) { + if (status.ok()) { + return status.ToString(); + } + std::string text; + if (!IsPrematureResponseError(status)) { + absl::StrAppend(&text, StatusCodeToString(ToEnvoyStatusCode(status.code())), ": ", + status.message()); + } else { + auto http_code = GetPrematureResponseHttpCode(status); + absl::StrAppend(&text, "PrematureResponseError: HTTP code: ", http_code, ": ", + status.message()); + } + return text; +} + +Status CodecProtocolError(absl::string_view message) { + return absl::Status(ToAbslStatusCode(StatusCode::CodecProtocolError), message); +} + +Status BufferFloodError(absl::string_view message) { + return absl::Status(ToAbslStatusCode(StatusCode::BufferFloodError), message); +} + +Status PrematureResponseError(absl::string_view message, Http::Code http_code) { + absl::Status status(ToAbslStatusCode(StatusCode::PrematureResponseError), message); + status.SetPayload( + EnvoyPayloadUrl, + absl::Cord(absl::string_view(reinterpret_cast(&http_code), sizeof(http_code)))); + return status; +} + +Status CodecClientError(absl::string_view message) { + return absl::Status(ToAbslStatusCode(StatusCode::CodecClientError), message); +} + +// Methods for checking and extracting error information +StatusCode GetStatusCode(const Status& status) { return ToEnvoyStatusCode(status.code()); } + +bool IsCodecProtocolError(const Status& status) { + return ToEnvoyStatusCode(status.code()) == StatusCode::CodecProtocolError; +} + +bool IsBufferFloodError(const Status& status) { + return ToEnvoyStatusCode(status.code()) == StatusCode::BufferFloodError; +} + +bool IsPrematureResponseError(const Status& status) { + return ToEnvoyStatusCode(status.code()) == StatusCode::PrematureResponseError; +} + +Http::Code GetPrematureResponseHttpCode(const Status& status) { + RELEASE_ASSERT(IsPrematureResponseError(status), "Must be PrematureResponseError"); + auto payload = status.GetPayload(EnvoyPayloadUrl); + RELEASE_ASSERT(payload.has_value(), "Must have payload"); + auto data = payload.value().Flatten(); + RELEASE_ASSERT(data.length() == sizeof(Http::Code), "Invalid payload length"); + return *reinterpret_cast(data.data()); +} + +bool IsCodecClientError(const Status& status) { + return ToEnvoyStatusCode(status.code()) == StatusCode::CodecClientError; +} + +} // namespace Envoy diff --git a/source/common/common/status.h b/source/common/common/status.h new file mode 100644 index 0000000000000..6f6df2ec28a8e --- /dev/null +++ b/source/common/common/status.h @@ -0,0 +1,94 @@ +#pragma once + +#include +#include + +#include "envoy/http/codes.h" + +#include "absl/status/status.h" +#include "absl/strings/string_view.h" + +/** + * Facility for returning rich error information. + * This facility is to be used in place of exceptions, in components where + * exceptions safety is not guaranteed (i.e. codecs). + * + * Envoy::Status is an alias of absl::Status. + * IMPORTANT: `absl::Status` constructor `absl::Status::code()` and absl::Status::ToString()` + * methods must not be used as they will not return correct error information. Instead the error + * value creating and corresponding error checking functions defined below must be used. + * TODO(yanavlasov): add clang-tidy or lint check to enforce this. + * + * Usage example: + * + * Envoy::Status Foo() { + * ... + * if (codec_error) { + * return CodecProtocolError("Invalid protocol"); + * } + * return Envoy::OkStatus(); + * } + * + * void Bar() { + * auto status = Foo(); + * if (status.ok()) { + * ... + * } else { + * ASSERT(IsCodecProtocolError(status)); + * ENVOY_LOG(debug, "Codec error encountered: {}", status.message()); + * } + * } + */ + +namespace Envoy { + +/** + * Status codes for representing classes of Envoy errors. + */ +enum class StatusCode : int { + Ok = 0, + CodecProtocolError = 1, + BufferFloodError = 2, + PrematureResponseError = 3, + CodecClientError = 4 +}; + +using Status = absl::Status; + +inline Status OkStatus() { return absl::OkStatus(); } + +/** + * Returns the combination of the error code name, message and any additional error attributes. + */ +std::string ToString(const Status& status); + +/** + * Functions for creating error values. The error code of the returned status object matches the + * name of the function. + */ +Status CodecProtocolError(absl::string_view message); +Status BufferFloodError(absl::string_view message); +Status PrematureResponseError(absl::string_view message, Http::Code http_code); +Status CodecClientError(absl::string_view message); + +/** + * Returns Envoy::StatusCode of the given status object. + * If the status object does not contain valid Envoy::Status value the function will RELEASE_ASSERT. + */ +ABSL_MUST_USE_RESULT StatusCode GetStatusCode(const Status& status); + +/** + * Returns true if the given status matches error code implied by the name of the function. + */ +ABSL_MUST_USE_RESULT bool IsCodecProtocolError(const Status& status); +ABSL_MUST_USE_RESULT bool IsBufferFloodError(const Status& status); +ABSL_MUST_USE_RESULT bool IsPrematureResponseError(const Status& status); +ABSL_MUST_USE_RESULT bool IsCodecClientError(const Status& status); + +/** + * Returns Http::Code value of the PrematureResponseError status. + * IsPrematureResponseError(status) must be true which is checked by RELEASE_ASSERT. + */ +ABSL_MUST_USE_RESULT Http::Code GetPrematureResponseHttpCode(const Status& status); + +} // namespace Envoy diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 22b4c6046c730..193decf644392 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -393,25 +393,3 @@ envoy_cc_library( "//source/common/common:logger_lib", ], ) - -envoy_cc_library( - name = "status_lib", - srcs = ["status.cc"], - hdrs = ["status.h"], - external_deps = [ - "abseil_base", - "abseil_optional", - ], - deps = [ - "//include/envoy/http:codes_interface", - ], -) - -envoy_cc_library( - name = "status_or_lib", - hdrs = ["status_or.h"], - external_deps = [ - "abseil_variant", - ], - deps = [":status_lib"], -) diff --git a/source/common/http/status.cc b/source/common/http/status.cc deleted file mode 100644 index e424596dc94c7..0000000000000 --- a/source/common/http/status.cc +++ /dev/null @@ -1,68 +0,0 @@ -#include "common/http/status.h" - -#include "absl/strings/str_cat.h" - -namespace Envoy { -namespace Http { - -namespace { - -std::string StatusCodeToString(StatusCode code) { - switch (code) { - default: - return ""; - case StatusCode::Ok: - return "OK"; - case StatusCode::CodecProtocolError: - return "CodecProtocolError"; - case StatusCode::BufferFloodError: - return "BufferFloodError"; - case StatusCode::PrematureResponse: - return "PrematureResponse"; - case StatusCode::CodecClientError: - return "CodecClientError"; - } -} - -} // namespace - -void Status::Unref() { - // Fast path: if ref==1, there is no need for a RefCountDec (since - // this is the only reference and therefore no other thread is - // allowed to be mucking with r). - if (rep_ && (rep_->ref_.load(std::memory_order_acquire) == 1 || - rep_->ref_.fetch_sub(1, std::memory_order_acq_rel) - 1 == 0)) { - delete rep_; - } -} - -Status::Status(StatusCode code, absl::string_view msg) - : rep_(code != StatusCode::Ok ? new StatusRep(code, msg) : nullptr) { - Ref(); -} - -Status::Status(StatusCode code, Http::Code http_code, absl::string_view msg) - : rep_(code != StatusCode::Ok ? new StatusRep(code, http_code, msg) : nullptr) { - Ref(); -} - -Status::~Status() { Unref(); } - -const std::string* Status::EmptyString() { - static auto* empty_string = new std::string; - return empty_string; -} - -std::string Status::ToStringError() const { - std::string text; - if (rep_->http_code_.has_value()) { - absl::StrAppend(&text, StatusCodeToString(Code()), ", HTTP code ", rep_->http_code_.value(), - ": ", Message()); - } else { - absl::StrAppend(&text, StatusCodeToString(Code()), ": ", Message()); - } - return text; -} - -} // namespace Http -} // namespace Envoy diff --git a/source/common/http/status.h b/source/common/http/status.h deleted file mode 100644 index 376f3887ae841..0000000000000 --- a/source/common/http/status.h +++ /dev/null @@ -1,148 +0,0 @@ -#pragma once - -#include -#include - -#include "envoy/http/codes.h" - -#include "absl/base/attributes.h" -#include "absl/strings/string_view.h" -#include "absl/types/optional.h" - -namespace Envoy { -namespace Http { - -/** - * Status codes for representing classes of codec errors. - */ -enum class StatusCode : int { - Ok = 0, - CodecProtocolError = 1, - BufferFloodError = 2, - PrematureResponse = 3, - CodecClientError = 4 -}; - -/** - * Class for returning error status. - * This class is to replace the use of exceptions in Envoy's codecs. - * Requirements: - * 1. Represent the type system of existing codec exceptions. - * 2. Highly efficient when the status has NO error. - * 3. Register size, to avoid stack allocations. - * 4. Extensible. - * - * Constructing an error status is likely as heavy as throwing an exception due to a heap - * allocation. - */ -class ABSL_MUST_USE_RESULT Status final { -public: - // Creates an OK status with no message. - Status() = default; - - /** - * Create a status with the specified code and - * error message. If `code == StatusCode::Ok`, `msg` is ignored and an - * object identical to an OK status is constructed. - */ - Status(StatusCode code, absl::string_view msg); - - /** - * Create a status with the specified code, HTTP code and - * error message. If `code == StatusCode::Ok`, `msg` and `http_code` are ignored and an - * object identical to an OK status is constructed. - */ - Status(StatusCode code, Http::Code http_code, absl::string_view msg); - - Status(const Status& other) : rep_(other.rep_) { Ref(); } - Status& operator=(const Status& x); - - // The moved-from state is the Ok. - Status(Status&& other) noexcept : rep_(other.rep_) { other.rep_ = nullptr; } - Status& operator=(Status&&) noexcept; - - ~Status(); - - // Returns true if the Status is OK. - ABSL_MUST_USE_RESULT bool Ok() const { return rep_ == nullptr; } - - // Returns the error code. - StatusCode Code() const { return Ok() ? StatusCode::Ok : rep_->status_code_; } - - /** - * Returns the error message. Note: prefer ToString() for debug logging. - * This message rarely describes the error code. It is not unusual for the - * error message to be the empty string. - */ - absl::string_view Message() const { return Ok() ? *EmptyString() : rep_->message_; } - - // Returns HTTP code if it was provided during Status construction - absl::optional HttpCode() const { - return Ok() ? absl::nullopt : rep_->http_code_; - } - - /** - * Returns a combination of the error code name, HTTP status if it was specified and the message. - * You can expect the code name and the message to be substrings of the - * result. - * WARNING: Do not depend on the exact format of the result of `ToString()` - * which is subject to change. - */ - std::string ToString() const { return Ok() ? "OK" : ToStringError(); } - - /** - * Ignores any errors. This method does nothing except potentially suppress - * complaints from any tools that are checking that errors are not dropped on - * the floor. - */ - void IgnoreError() const {} - -private: - static const std::string* EmptyString(); - - // Returns string for non-ok Status. - std::string ToStringError() const; - - struct StatusRep { - StatusRep(StatusCode code, absl::string_view msg) - : ref_(0), status_code_(code), message_(msg) {} - StatusRep(StatusCode code, Http::Code http_code, absl::string_view msg) - : ref_(0), status_code_(code), message_(msg), http_code_(http_code) {} - - std::atomic ref_; - StatusCode status_code_; - std::string message_; - absl::optional http_code_; - }; - - void Ref() { - if (rep_) { - rep_->ref_.fetch_add(1, std::memory_order_relaxed); - } - } - void Unref(); - - StatusRep* rep_{nullptr}; -}; - -inline Status& Status::operator=(const Status& other) { // NOLINT self assignment - if (other.rep_ != rep_) { - Unref(); - rep_ = other.rep_; - Ref(); - } - return *this; -} - -inline Status& Status::operator=(Status&& other) noexcept { - Unref(); - rep_ = other.rep_; - Ref(); - return *this; -} - -// Helper to make code returning Ok status more readable. -inline Status OkStatus() { return Status(); } - -} // namespace Http -} // namespace Envoy diff --git a/source/common/http/status_or.h b/source/common/http/status_or.h deleted file mode 100644 index cf9d4cf163f08..0000000000000 --- a/source/common/http/status_or.h +++ /dev/null @@ -1,144 +0,0 @@ -#pragma once - -#include "common/http/status.h" - -#include "absl/types/variant.h" - -namespace Envoy { -namespace Http { - -/** - * Union of the Status and a T object. - * An object of StatusOr has either usable value or an error - * explaining why the value is not present. - * This class is used to facilitate replacement of exceptions with - * a return code in HTTP codecs. For instance method Foo() that throws an exception and returns an - * int - * - * int Foo() { - * ... - * if (error_condition) { - * throw CodecProtocolException("Error"); - * } - * return valid_value; - * } - * - * can be replaced with: - * - * StatusOr Foo() { - * ... - * if (error_condition) { - * return Status(StatusCode::CodecProtocolError, "Error"); - * } - * return valid_value; - * } - */ -template class StatusOr { - using StatusOrValue = absl::variant; - -public: - /** - * Construct a new StatusOr with the given non-ok status. After calling - * this constructor, calls to Value() will RELEASE_ASSERT-fail. - * - * NOTE: Not explicit - we want to use StatusOr as a return - * value, so it is convenient and sensible to be able to do 'return - * Status(...)' when the return type is StatusOr. - * - * REQUIRES: status != StatusCode::Ok(). This requirement is checked with RELEASE_ASSERT. - */ - StatusOr(const Status& status) : status_or_value_(status) { - RELEASE_ASSERT(!status.Ok(), "Status must not be Ok"); - } - - /** - * Construct a new StatusOr with the given value. - * After calling this constructor, calls to - * Value() will succeed, and calls to Status() will return OK. - * - * NOTE: Not explicit - we want to use StatusOr as a return type - * so it is convenient and sensible to be able to do 'return T()' - * when when the return type is StatusOr. - */ - StatusOr(const T& value) : status_or_value_(value) {} - StatusOr(T&& value) : status_or_value_(std::move(value)) {} - - /** - * Conversion copy constructor and assignment operator, T must be copy constructible from U - * TODO(yanavlasov): disable this constructor for cases where T can be copy constructed from - * StatusOr to avoid ambiguity. - */ - template - StatusOr(const StatusOr& other) - : status_or_value_(other.Ok() ? StatusOrValue(other.Value()) - : StatusOrValue(other.Status())) {} - template StatusOr& operator=(const StatusOr& other) { - status_or_value_ = other.Ok() ? StatusOrValue(other.Value()) : StatusOrValue(other.Status()); - return *this; - } - - /** - * Move constructor is a bit weird. Move operation must leave the moved out of object in a valid - * state. However moved out of Status is in the Ok() state, which makes the StatusOr to be in an - * invalid state, there status is Ok but there is no value. As such move operations move the value - * BUT copy the status, such that the moved out of StatusOr remains valid when it carries Status. - * It is a bit inefficient for error cases, but still allows T to be a move only type - */ - StatusOr(StatusOr&& other) noexcept - : status_or_value_(other.Ok() ? StatusOrValue(std::move(other).Value()) - : StatusOrValue(other.Status())) {} - StatusOr& operator=(StatusOr&& other) noexcept { - status_or_value_ = - other.Ok() ? StatusOrValue(std::move(other).Value()) : StatusOrValue(other.Status()); - return *this; - } - StatusOr(const StatusOr&) = default; - StatusOr& operator=(const StatusOr&) = default; - - template - StatusOr(StatusOr&& other) - : status_or_value_(other.Ok() ? StatusOrValue(std::move(other).Value()) - : StatusOrValue(other.Status())) {} - template StatusOr& operator=(StatusOr&& other) { - status_or_value_ = - other.Ok() ? StatusOrValue(std::move(other).Value()) : StatusOrValue(other.Status()); - return *this; - } - - /** - * Returns a reference to our status. If this contains a T, then - * returns Status::OK. - */ - const Status& Status() const { - static const class Status ok_status; - return Ok() ? ok_status : absl::get(status_or_value_); - } - - bool Ok() const { return absl::holds_alternative(status_or_value_); } - - // Returns various kinds of references to the current value, or RELEASE_ASSERT if !this->ok(). - const T& Value() const& { - RELEASE_ASSERT(Ok(), "Status must be Ok"); - return absl::get(status_or_value_); - } - T& Value() & { - RELEASE_ASSERT(Ok(), "Status must be Ok"); - return absl::get(status_or_value_); - } - - // Support move operation on underlying value. Invoke using `std::move(status_or).Value()` - T&& Value() && { - RELEASE_ASSERT(Ok(), "Status must be Ok"); - return absl::get(std::move(status_or_value_)); - } - const T&& Value() const&& { - RELEASE_ASSERT(Ok(), "Status must be Ok"); - return absl::get(std::move(status_or_value_)); - } - -private: - StatusOrValue status_or_value_; -}; - -} // namespace Http -} // namespace Envoy diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 8dbb40bca2f3b..9e7cb3dcd9ca9 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -265,3 +265,11 @@ envoy_cc_test( "//source/common/common:version_lib", ], ) + +envoy_cc_test( + name = "status_test", + srcs = ["status_test.cc"], + deps = [ + "//source/common/common:status_lib", + ], +) diff --git a/test/common/common/status_test.cc b/test/common/common/status_test.cc new file mode 100644 index 0000000000000..15bc0860873f1 --- /dev/null +++ b/test/common/common/status_test.cc @@ -0,0 +1,69 @@ +#include "common/common/status.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { + +TEST(Status, Ok) { + auto status = OkStatus(); + EXPECT_TRUE(status.ok()); + EXPECT_TRUE(status.message().empty()); + EXPECT_EQ("OK", ToString(status)); + EXPECT_EQ(StatusCode::Ok, GetStatusCode(status)); + EXPECT_FALSE(IsCodecProtocolError(status)); + EXPECT_FALSE(IsBufferFloodError(status)); + EXPECT_FALSE(IsPrematureResponseError(status)); + EXPECT_FALSE(IsCodecClientError(status)); +} + +TEST(Status, CodecProtocolError) { + auto status = CodecProtocolError("foobar"); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("foobar", status.message()); + EXPECT_EQ("CodecProtocolError: foobar", ToString(status)); + EXPECT_EQ(StatusCode::CodecProtocolError, GetStatusCode(status)); + EXPECT_TRUE(IsCodecProtocolError(status)); + EXPECT_FALSE(IsBufferFloodError(status)); + EXPECT_FALSE(IsPrematureResponseError(status)); + EXPECT_FALSE(IsCodecClientError(status)); +} + +TEST(Status, BufferFloodError) { + auto status = BufferFloodError("foobar"); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("foobar", status.message()); + EXPECT_EQ("BufferFloodError: foobar", ToString(status)); + EXPECT_EQ(StatusCode::BufferFloodError, GetStatusCode(status)); + EXPECT_FALSE(IsCodecProtocolError(status)); + EXPECT_TRUE(IsBufferFloodError(status)); + EXPECT_FALSE(IsPrematureResponseError(status)); + EXPECT_FALSE(IsCodecClientError(status)); +} + +TEST(Status, PrematureResponseError) { + auto status = PrematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("foobar", status.message()); + EXPECT_EQ("PrematureResponseError: HTTP code: 407: foobar", ToString(status)); + EXPECT_EQ(StatusCode::PrematureResponseError, GetStatusCode(status)); + EXPECT_FALSE(IsCodecProtocolError(status)); + EXPECT_FALSE(IsBufferFloodError(status)); + EXPECT_TRUE(IsPrematureResponseError(status)); + EXPECT_EQ(Http::Code::ProxyAuthenticationRequired, GetPrematureResponseHttpCode(status)); + EXPECT_FALSE(IsCodecClientError(status)); +} + +TEST(Status, CodecClientError) { + auto status = CodecClientError("foobar"); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("foobar", status.message()); + EXPECT_EQ("CodecClientError: foobar", ToString(status)); + EXPECT_EQ(StatusCode::CodecClientError, GetStatusCode(status)); + EXPECT_FALSE(IsCodecProtocolError(status)); + EXPECT_FALSE(IsBufferFloodError(status)); + EXPECT_FALSE(IsPrematureResponseError(status)); + EXPECT_TRUE(IsCodecClientError(status)); +} + +} // namespace Envoy diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 55429af6783d2..c7dadc09bc493 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -368,19 +368,3 @@ envoy_cc_test( "//source/common/http:path_utility_lib", ], ) - -envoy_cc_test( - name = "status_test", - srcs = ["status_test.cc"], - deps = [ - "//source/common/http:status_lib", - ], -) - -envoy_cc_test( - name = "status_or_test", - srcs = ["status_or_test.cc"], - deps = [ - "//source/common/http:status_or_lib", - ], -) diff --git a/test/common/http/status_or_test.cc b/test/common/http/status_or_test.cc deleted file mode 100644 index 002cb76e0cfcc..0000000000000 --- a/test/common/http/status_or_test.cc +++ /dev/null @@ -1,135 +0,0 @@ -#include - -#include "common/http/status_or.h" - -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -namespace Envoy { -namespace Http { - -TEST(StatusOr, Status) { - StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); - EXPECT_FALSE(status_or.Ok()); - EXPECT_FALSE(status_or.Status().Ok()); - EXPECT_EQ(StatusCode::CodecProtocolError, status_or.Status().Code()); - EXPECT_EQ("foobar", status_or.Status().Message()); -} - -TEST(StatusOr, CopyStatus) { - StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); - StatusOr status_or_copy(status_or); // NOLINT - unit test - EXPECT_FALSE(status_or_copy.Ok()); - EXPECT_FALSE(status_or_copy.Status().Ok()); - EXPECT_EQ(StatusCode::CodecProtocolError, status_or_copy.Status().Code()); - EXPECT_EQ("foobar", status_or_copy.Status().Message()); -} - -TEST(StatusOr, CopyErrorIntoValue) { - StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); - StatusOr status_or_copy(1234); - status_or_copy = status_or; - EXPECT_FALSE(status_or_copy.Ok()); - EXPECT_FALSE(status_or_copy.Status().Ok()); - EXPECT_EQ(StatusCode::CodecProtocolError, status_or_copy.Status().Code()); - EXPECT_EQ("foobar", status_or_copy.Status().Message()); -} - -TEST(StatusOr, MoveStatus) { - StatusOr status_or = Status(StatusCode::CodecProtocolError, "foobar"); - StatusOr status_or_copy(std::move(status_or)); - - // status_or should retain the error status - EXPECT_FALSE(status_or.Ok()); // NOLINT - unit test - EXPECT_FALSE(status_or.Status().Ok()); - EXPECT_EQ(StatusCode::CodecProtocolError, status_or.Status().Code()); - EXPECT_EQ("foobar", status_or.Status().Message()); - - EXPECT_FALSE(status_or_copy.Ok()); - EXPECT_FALSE(status_or_copy.Status().Ok()); - EXPECT_EQ(StatusCode::CodecProtocolError, status_or_copy.Status().Code()); - EXPECT_EQ("foobar", status_or_copy.Status().Message()); -} - -TEST(StatusOr, Value) { - StatusOr status_or = 55; - EXPECT_TRUE(status_or.Ok()); - EXPECT_TRUE(status_or.Status().Ok()); - EXPECT_EQ(StatusCode::Ok, status_or.Status().Code()); - EXPECT_EQ("", status_or.Status().Message()); - EXPECT_EQ(55, status_or.Value()); -} - -TEST(StatusOr, CopyValue) { - StatusOr status_or = 55; - StatusOr status_or_copy = status_or; - EXPECT_TRUE(status_or_copy.Ok()); - EXPECT_TRUE(status_or_copy.Status().Ok()); - EXPECT_EQ(StatusCode::Ok, status_or_copy.Status().Code()); - EXPECT_EQ("", status_or_copy.Status().Message()); - EXPECT_EQ(55, status_or_copy.Value()); -} - -TEST(StatusOr, CopyValueIntoError) { - StatusOr status_or = 55; - StatusOr status_or_copy(Status(StatusCode::CodecProtocolError, "foobar")); - status_or_copy = status_or; - EXPECT_TRUE(status_or_copy.Ok()); - EXPECT_TRUE(status_or_copy.Status().Ok()); - EXPECT_EQ(StatusCode::Ok, status_or_copy.Status().Code()); - EXPECT_EQ("", status_or_copy.Status().Message()); - EXPECT_EQ(55, status_or_copy.Value()); -} - -// Validate that StatusOr works with move only types -TEST(StatusOr, MoveOnlyValue) { - StatusOr> status_or(std::make_unique(55)); - EXPECT_TRUE(status_or.Ok()); - EXPECT_TRUE(status_or.Status().Ok()); - EXPECT_EQ(StatusCode::Ok, status_or.Status().Code()); - EXPECT_EQ("", status_or.Status().Message()); - EXPECT_EQ(55, *status_or.Value()); - - StatusOr> another_status_or(std::move(status_or)); - EXPECT_EQ(55, *another_status_or.Value()); - - std::unique_ptr value(std::move(another_status_or).Value()); - EXPECT_EQ(55, *value); - EXPECT_EQ(nullptr, another_status_or.Value()); // NOLINT - unit test - EXPECT_TRUE(another_status_or.Ok()); -} - -TEST(StatusOr, CopyConversion) { - struct A { - A(int m) : m_(m) {} - int m_{}; - }; - struct B { - B(const A& a) : n_(10 + a.m_) {} - int n_{}; - }; - - StatusOr a = A(12); - StatusOr b(a); - EXPECT_EQ(22, b.Value().n_); -} - -TEST(StatusOr, MoveConversion) { - struct A { - A(int m) : m_(m) {} - virtual ~A() = default; - int m_{}; - }; - struct B : public A { - B(int n) : A(10 + n), n_(n) {} - int n_{}; - }; - - StatusOr> b(std::make_unique(121)); - StatusOr> a(std::move(b)); - EXPECT_EQ(131, a.Value()->m_); - EXPECT_EQ(nullptr, b.Value()); // NOLINT - unit test -} - -} // namespace Http -} // namespace Envoy diff --git a/test/common/http/status_test.cc b/test/common/http/status_test.cc deleted file mode 100644 index c01bd4ee923c5..0000000000000 --- a/test/common/http/status_test.cc +++ /dev/null @@ -1,112 +0,0 @@ -#include "common/http/status.h" - -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -namespace Envoy { -namespace Http { - -TEST(Status, Defaults) { - Status status; - EXPECT_EQ(sizeof(void*), sizeof(status)); - EXPECT_TRUE(status.Ok()); - EXPECT_EQ(StatusCode::Ok, status.Code()); - EXPECT_FALSE(status.HttpCode().has_value()); - EXPECT_TRUE(status.Message().empty()); - EXPECT_EQ("OK", status.ToString()); -} - -TEST(Status, OkStatusIgnoresOtherConstructorParameters) { - Status status(StatusCode::Ok, "foobar"); - EXPECT_TRUE(status.Ok()); - EXPECT_EQ(StatusCode::Ok, status.Code()); - EXPECT_FALSE(status.HttpCode().has_value()); - EXPECT_TRUE(status.Message().empty()); - EXPECT_EQ("OK", status.ToString()); - - Status another_status(StatusCode::Ok, Http::Code::InternalServerError, "foobar"); - EXPECT_TRUE(another_status.Ok()); - EXPECT_EQ(StatusCode::Ok, another_status.Code()); - EXPECT_FALSE(another_status.HttpCode().has_value()); - EXPECT_TRUE(another_status.Message().empty()); - EXPECT_EQ("OK", another_status.ToString()); -} - -TEST(Status, ErrorAndMessage) { - Status status(StatusCode::CodecProtocolError, "foobar"); - EXPECT_FALSE(status.Ok()); - EXPECT_EQ(StatusCode::CodecProtocolError, status.Code()); - EXPECT_FALSE(status.HttpCode().has_value()); - EXPECT_EQ("foobar", status.Message()); - EXPECT_EQ("CodecProtocolError: foobar", status.ToString()); -} - -TEST(Status, ErrorHttpCodeAndMessage) { - Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); - EXPECT_FALSE(status.Ok()); - EXPECT_EQ(StatusCode::BufferFloodError, status.Code()); - EXPECT_TRUE(status.HttpCode().has_value()); - EXPECT_EQ(Http::Code::BadRequest, status.HttpCode().value()); - EXPECT_EQ("foobar", status.Message()); - EXPECT_EQ("BufferFloodError, HTTP code 400: foobar", status.ToString()); -} - -TEST(Status, MoveOkIntoOk) { - Status status; - Status another_status(std::move(status)); - EXPECT_TRUE(status.Ok()); // NOLINT - unit test - EXPECT_TRUE(another_status.Ok()); -} - -TEST(Status, MoveOkIntoError) { - Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); - Status another_status; - status = std::move(another_status); - EXPECT_TRUE(status.Ok()); - EXPECT_TRUE(another_status.Ok()); // NOLINT - unit test -} - -TEST(Status, MoveErrorIntoOk) { - Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); - Status another_status(std::move(status)); - EXPECT_TRUE(status.Ok()); // NOLINT - unit test - EXPECT_FALSE(another_status.Ok()); - EXPECT_EQ(StatusCode::BufferFloodError, another_status.Code()); - EXPECT_TRUE(another_status.HttpCode().has_value()); - EXPECT_EQ(Http::Code::BadRequest, another_status.HttpCode().value()); - EXPECT_EQ("foobar", another_status.Message()); -} - -TEST(Status, CopyOkIntoOk) { - Status status; - Status another_status(status); // NOLINT - unit test - EXPECT_TRUE(status.Ok()); - EXPECT_TRUE(another_status.Ok()); -} - -TEST(Status, CopyOkIntoError) { - Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); - Status another_status; - status = another_status; - EXPECT_TRUE(status.Ok()); - EXPECT_TRUE(another_status.Ok()); -} - -TEST(Status, CopyErrorIntoOk) { - Status status(StatusCode::BufferFloodError, Http::Code::BadRequest, "foobar"); - Status another_status(status); // NOLINT - unit test - EXPECT_FALSE(status.Ok()); - EXPECT_EQ(StatusCode::BufferFloodError, status.Code()); - EXPECT_TRUE(status.HttpCode().has_value()); - EXPECT_EQ(Http::Code::BadRequest, status.HttpCode().value()); - EXPECT_EQ("foobar", status.Message()); - - EXPECT_FALSE(another_status.Ok()); - EXPECT_EQ(StatusCode::BufferFloodError, another_status.Code()); - EXPECT_TRUE(another_status.HttpCode().has_value()); - EXPECT_EQ(Http::Code::BadRequest, another_status.HttpCode().value()); - EXPECT_EQ("foobar", another_status.Message()); -} - -} // namespace Http -} // namespace Envoy From 01d1739ab9081b3a861db3e86c677ec92bdb9564 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Mon, 6 Apr 2020 16:16:17 -0400 Subject: [PATCH 04/11] Fork absl::StatusOr Signed-off-by: Yan Avlasov --- source/common/common/BUILD | 15 + source/common/common/status.h | 2 +- source/common/common/statusor.cc | 38 ++ source/common/common/statusor.h | 378 +++++++++++++++++++ source/common/common/statusor_internals.h | 244 ++++++++++++ test/common/common/BUILD | 9 + test/common/common/statusor_test.cc | 440 ++++++++++++++++++++++ tools/spelling/spelling_dictionary.txt | 1 + 8 files changed, 1126 insertions(+), 1 deletion(-) create mode 100644 source/common/common/statusor.cc create mode 100644 source/common/common/statusor.h create mode 100644 source/common/common/statusor_internals.h create mode 100644 test/common/common/statusor_test.cc diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 264aa8314e9b7..43e74a1cfd674 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -441,3 +441,18 @@ envoy_cc_library( "//include/envoy/http:codes_interface", ], ) + +envoy_cc_library( + name = "statusor_lib", + srcs = [ + "statusor.cc", + ], + hdrs = [ + "statusor.h", + "statusor_internals.h", + ], + external_deps = [ + "abseil_status", + ], + deps = [":assert_lib"], +) diff --git a/source/common/common/status.h b/source/common/common/status.h index 6f6df2ec28a8e..80aaa2a4c02d6 100644 --- a/source/common/common/status.h +++ b/source/common/common/status.h @@ -75,7 +75,7 @@ Status CodecClientError(absl::string_view message); * Returns Envoy::StatusCode of the given status object. * If the status object does not contain valid Envoy::Status value the function will RELEASE_ASSERT. */ -ABSL_MUST_USE_RESULT StatusCode GetStatusCode(const Status& status); +StatusCode GetStatusCode(const Status& status); /** * Returns true if the given status matches error code implied by the name of the function. diff --git a/source/common/common/statusor.cc b/source/common/common/statusor.cc new file mode 100644 index 0000000000000..bc9782aa7594b --- /dev/null +++ b/source/common/common/statusor.cc @@ -0,0 +1,38 @@ +/* + * Copyright 2019 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "common/common/statusor.h" + +#include + +#include "common/common/assert.h" + +namespace Envoy { + +namespace StatusOr_Internal { + +void Helper::HandleInvalidStatusCtorArg(absl::Status* status) { + const char* kMessage = "An OK status is not a valid constructor argument to StatusOr"; + ASSERT(false, kMessage); + // In optimized builds, we will fall back to ::util::error::INTERNAL. + *status = absl::Status(absl::StatusCode::kInternal, kMessage); +} + +void Helper::Crash(const absl::Status&) { abort(); } + +} // namespace StatusOr_Internal + +} // namespace Envoy diff --git a/source/common/common/statusor.h b/source/common/common/statusor.h new file mode 100644 index 0000000000000..121b1274e665f --- /dev/null +++ b/source/common/common/statusor.h @@ -0,0 +1,378 @@ +/** + * IMPORTANT: this file is a fork of the soon to be open-source absl::StatusOr class. + * When the absl::StatusOr lands this file will just define Envoy::StatusOr as an alias + * of absl::StatusOr. + * + * IMPORTANT: StatusOr default constructor must not be used as it does not fit intro any + * Envoy's use cases. The error extracting functions in the common/common/status.h will + * RELEASE_ASSERT on default initialized StatusOr. + * Usage example: + * + * Envoy::StatusOr Foo() { + * ... + * if (codec_error) { + * return CodecProtocolError("Invalid protocol"); + * } + * return 123456; + * } + * + * void Bar() { + * auto status_or = Foo(); + * if (status_or.ok()) { + * int result = status_or.ValueOrDie(); + * ... + * } else { + * ASSERT(IsCodecProtocolError(status_or.status())); + * ENVOY_LOG(debug, "Codec error encountered: {}", status_or.status().message()); + * } + * } + + */ + +/* + * Copyright 2019 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// StatusOr is the union of a Status object and a T +// object. StatusOr models the concept of an object that is either a +// usable value, or an error Status explaining why such a value is +// not present. To this end, StatusOr does not allow its Status +// value to be OkStatus(). +// +// The primary use-case for StatusOr is as the return value of a +// function which may fail. +// +// Example usage of a StatusOr: +// +// StatusOr result = DoBigCalculationThatCouldFail(); +// if (result) { +// result->DoSomethingCool(); +// } else { +// GOOGLE_LOG(ERROR) << result.status(); +// } +// +// Example that is guaranteed crash if the result holds no value: +// +// StatusOr result = DoBigCalculationThatCouldFail(); +// const Foo& foo = result.ValueOrDie(); +// foo.DoSomethingCool(); +// +// Example usage of a StatusOr>: +// +// StatusOr> result = FooFactory::MakeNewFoo(arg); +// if (!result) { +// GOOGLE_LOG(ERROR) << result.status(); +// } else if (*result == nullptr) { +// GOOGLE_LOG(ERROR) << "Unexpected null pointer"; +// } else { +// (*result)->DoSomethingCool(); +// } +// +// Example factory implementation returning StatusOr: +// +// StatusOr FooFactory::MakeFoo(int arg) { +// if (arg <= 0) { +// return ::cel_base::Status(::cel_base::INVALID_ARGUMENT, +// "Arg must be positive"); +// } +// return Foo(arg); +// } +// + +#include +#include +#include +#include + +#include "common/common/statusor_internals.h" + +#include "absl/base/attributes.h" +#include "absl/base/macros.h" + +namespace Envoy { + +// Returned StatusOr objects may not be ignored. +template class ABSL_MUST_USE_RESULT StatusOr; + +template +class StatusOr : private StatusOr_Internal::StatusOrData, + private StatusOr_Internal::TraitsBase::value, + std::is_move_constructible::value> { + template friend class StatusOr; + + typedef StatusOr_Internal::StatusOrData Base; + +public: + using element_type = T; + + // Constructs a new StatusOr with Status::UNKNOWN status. This is marked + // 'explicit' to try to catch cases like 'return {};', where people think + // StatusOr> will be initialized with an empty vector, + // instead of a Status::UNKNOWN status. + explicit StatusOr(); + + // StatusOr will be copy constructible/assignable if T is copy + // constructible. + StatusOr(const StatusOr&) = default; + StatusOr& operator=(const StatusOr&) = default; + + // StatusOr will be move constructible/assignable if T is move + // constructible. + StatusOr(StatusOr&&) = default; + StatusOr& operator=(StatusOr&&) = default; + + // Conversion copy/move constructor, T must be convertible from U. + // These should not participate in overload resolution if U + // is not convertible to T. + template StatusOr(const StatusOr& other); + template StatusOr(StatusOr&& other); + + // Conversion copy/move assignment operator, T must be convertible from U. + template StatusOr& operator=(const StatusOr& other); + template StatusOr& operator=(StatusOr&& other); + + // Constructs a new StatusOr with the given value. After calling this + // constructor, this->ok() will be true and the contained value may be + // retrieved with ValueOrDie(), operator*(), or operator->(). + // + // NOTE: Not explicit - we want to use StatusOr as a return type + // so it is convenient and sensible to be able to do 'return T()' + // when the return type is StatusOr. + // + // REQUIRES: T is copy constructible. + StatusOr(const T& value); + + // Constructs a new StatusOr with the given non-ok status. After calling this + // constructor, this->ok() will be false and calls to ValueOrDie() will + // CHECK-fail. + // + // NOTE: Not explicit - we want to use StatusOr as a return + // value, so it is convenient and sensible to be able to do 'return + // Status()' when the return type is StatusOr. + // + // REQUIRES: !status.ok(). This requirement is checked by ASSERT. + // In optimized builds, passing OkStatus() here will have the effect + // of passing INTERNAL as a fallback. + StatusOr(const absl::Status& status); + StatusOr& operator=(const absl::Status& status); + + // Similar to the `const T&` overload. + // + // REQUIRES: T is move constructible. + StatusOr(T&& value); + + // RValue versions of the operations declared above. + StatusOr(absl::Status&& status); + StatusOr& operator=(absl::Status&& status); + + // Returns this->ok() + explicit operator bool() const { return ok(); } + + // Returns this->status().ok() + ABSL_MUST_USE_RESULT bool ok() const { return this->status_.ok(); } + + // Returns a reference to our status. If this contains a T, then + // returns OkStatus(). + const absl::Status& status() const&; + absl::Status status() &&; + + // Returns a reference to our current value, or ASSERT-fails if !this->ok(). If + // you have already checked the status using this->ok() or operator bool(), + // then you probably want to use operator*() or operator->() to access the + // current value instead of ValueOrDie(). + // + // Note: for value types that are cheap to copy, prefer simple code: + // + // T value = status_or.ValueOrDie(); + // + // Otherwise, if the value type is expensive to copy, but can be left + // in the StatusOr, simply assign to a reference: + // + // T& value = status_or.ValueOrDie(); // or `const T&` + // + // Otherwise, if the value type supports an efficient move, it can be + // used as follows: + // + // T value = std::move(status_or).ValueOrDie(); + // + // The std::move on status_or instead of on the whole expression enables + // warnings about possible uses of the status_or object after the move. + + const T& ValueOrDie() const&; + T& ValueOrDie() &; + const T&& ValueOrDie() const&&; + T&& ValueOrDie() &&; + + // Returns a reference to the current value. + // + // REQUIRES: this->ok() == true, otherwise the behavior is undefined. + // + // Use this->ok() or `operator bool()` to verify that there is a current + // value. Alternatively, see ValueOrDie() for a similar API that guarantees + // ASSERT-failing if there is no current value. + const T& operator*() const&; + T& operator*() &; + const T&& operator*() const&&; + T&& operator*() &&; + + // Returns a pointer to the current value. + // + // REQUIRES: this->ok() == true, otherwise the behavior is undefined. + // + // Use this->ok() or `operator bool()` to verify that there is a current + // value. + const T* operator->() const; + T* operator->(); + + // Returns a copy of the current value if this->ok() == true. Otherwise + // returns a default value. + template T value_or(U&& default_value) const&; + template T value_or(U&& default_value) &&; + + // Ignores any errors. This method does nothing except potentially suppress + // complaints from any tools that are checking that errors are not dropped on + // the floor. + void IgnoreError() const; +}; + +//////////////////////////////////////////////////////////////////////////////// +// Implementation details for StatusOr + +template +StatusOr::StatusOr() : Base(absl::Status(absl::StatusCode::kUnknown, "")) {} + +template StatusOr::StatusOr(const T& value) : Base(value) {} + +template StatusOr::StatusOr(const absl::Status& status) : Base(status) {} + +template StatusOr& StatusOr::operator=(const absl::Status& status) { + this->Assign(status); + return *this; +} + +template StatusOr::StatusOr(T&& value) : Base(std::move(value)) {} + +template StatusOr::StatusOr(absl::Status&& status) : Base(std::move(status)) {} + +template StatusOr& StatusOr::operator=(absl::Status&& status) { + this->Assign(std::move(status)); + return *this; +} + +template +template +inline StatusOr::StatusOr(const StatusOr& other) + : Base(static_cast::Base&>(other)) {} + +template +template +inline StatusOr& StatusOr::operator=(const StatusOr& other) { + if (other.ok()) + this->Assign(other.ValueOrDie()); + else + this->Assign(other.status()); + return *this; +} + +template +template +inline StatusOr::StatusOr(StatusOr&& other) + : Base(static_cast::Base&&>(other)) {} + +template +template +inline StatusOr& StatusOr::operator=(StatusOr&& other) { + if (other.ok()) { + this->Assign(std::move(other).ValueOrDie()); + } else { + this->Assign(std::move(other).status()); + } + return *this; +} + +template const absl::Status& StatusOr::status() const& { return this->status_; } +template absl::Status StatusOr::status() && { + return ok() ? absl::OkStatus() : std::move(this->status_); +} + +template const T& StatusOr::ValueOrDie() const& { + this->EnsureOk(); + return this->data_; +} + +template T& StatusOr::ValueOrDie() & { + this->EnsureOk(); + return this->data_; +} + +template const T&& StatusOr::ValueOrDie() const&& { + this->EnsureOk(); + return std::move(this->data_); +} + +template T&& StatusOr::ValueOrDie() && { + this->EnsureOk(); + return std::move(this->data_); +} + +template const T& StatusOr::operator*() const& { + this->EnsureOk(); + return this->data_; +} + +template T& StatusOr::operator*() & { + this->EnsureOk(); + return this->data_; +} + +template const T&& StatusOr::operator*() const&& { + this->EnsureOk(); + return std::move(this->data_); +} + +template T&& StatusOr::operator*() && { + this->EnsureOk(); + return std::move(this->data_); +} + +template const T* StatusOr::operator->() const { + this->EnsureOk(); + return &this->data_; +} + +template T* StatusOr::operator->() { + this->EnsureOk(); + return &this->data_; +} + +template template T StatusOr::value_or(U&& default_value) const& { + if (ok()) { + return this->data_; + } + return std::forward(default_value); +} + +template template T StatusOr::value_or(U&& default_value) && { + if (ok()) { + return std::move(this->data_); + } + return std::forward(default_value); +} + +template void StatusOr::IgnoreError() const { + // no-op +} + +} // namespace Envoy diff --git a/source/common/common/statusor_internals.h b/source/common/common/statusor_internals.h new file mode 100644 index 0000000000000..8c7205322bddb --- /dev/null +++ b/source/common/common/statusor_internals.h @@ -0,0 +1,244 @@ +/** + * IMPORTANT: this file is a fork of the soon to be open-source absl::StatusOr class. + * When the absl::StatusOr lands this file will be removed. + */ + +/* + * Copyright 2019 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include + +#include "absl/base/attributes.h" +#include "absl/meta/type_traits.h" +#include "absl/status/status.h" + +namespace Envoy { + +namespace StatusOr_Internal { + +class Helper { +public: + // Move type-agnostic error handling to the .cc. + static void HandleInvalidStatusCtorArg(absl::Status*); + ABSL_ATTRIBUTE_NORETURN static void Crash(const absl::Status& status); +}; + +// Construct an instance of T in `p` through placement new, passing Args... to +// the constructor. +// This abstraction is here mostly for the gcc performance fix. +template void PlacementNew(void* p, Args&&... args) { +#if defined(__GNUC__) && !defined(__clang__) + // Teach gcc that 'p' cannot be null, fixing code size issues. + if (p == nullptr) + __builtin_unreachable(); +#endif + new (p) T(std::forward(args)...); +} + +// Helper base class to hold the data and all operations. +// We move all this to a base class to allow mixing with the appropriate +// TraitsBase specialization. +template class StatusOrData { + template friend class StatusOrData; + +public: + StatusOrData() = delete; + + StatusOrData(const StatusOrData& other) { + if (other.ok()) { + MakeValue(other.data_); + MakeStatus(); + } else { + MakeStatus(other.status_); + } + } + + StatusOrData(StatusOrData&& other) noexcept { + if (other.ok()) { + MakeValue(std::move(other.data_)); + MakeStatus(); + } else { + MakeStatus(std::move(other.status_)); + } + } + + template StatusOrData(const StatusOrData& other) { + if (other.ok()) { + MakeValue(other.data_); + MakeStatus(); + } else { + MakeStatus(other.status_); + } + } + + template StatusOrData(StatusOrData&& other) { + if (other.ok()) { + MakeValue(std::move(other.data_)); + MakeStatus(); + } else { + MakeStatus(std::move(other.status_)); + } + } + + explicit StatusOrData(const T& value) : data_(value) { MakeStatus(); } + explicit StatusOrData(T&& value) : data_(std::move(value)) { MakeStatus(); } + + explicit StatusOrData(const absl::Status& status) : status_(status) { EnsureNotOk(); } + explicit StatusOrData(absl::Status&& status) : status_(std::move(status)) { EnsureNotOk(); } + + StatusOrData& operator=(const StatusOrData& other) { + if (this == &other) + return *this; + if (other.ok()) + Assign(other.data_); + else + Assign(other.status_); + return *this; + } + + StatusOrData& operator=(StatusOrData&& other) { + if (this == &other) + return *this; + if (other.ok()) + Assign(std::move(other.data_)); + else + Assign(std::move(other.status_)); + return *this; + } + + ~StatusOrData() { + if (ok()) { + status_.~Status(); + data_.~T(); + } else { + status_.~Status(); + } + } + + void Assign(const T& value) { + if (ok()) { + data_.~T(); + MakeValue(value); + } else { + MakeValue(value); + status_ = absl::OkStatus(); + } + } + + void Assign(T&& value) { + if (ok()) { + data_.~T(); + MakeValue(std::move(value)); + } else { + MakeValue(std::move(value)); + status_ = absl::OkStatus(); + } + } + + void Assign(const absl::Status& status) { + Clear(); + status_ = status; + EnsureNotOk(); + } + + void Assign(absl::Status&& status) { + Clear(); + status_ = std::move(status); + EnsureNotOk(); + } + + bool ok() const { return status_.ok(); } + +protected: + // status_ will always be active after the constructor. + // We make it a union to be able to initialize exactly how we need without + // waste. + // E/g. in the copy constructor we use the default constructor of + // Status in the ok() path to avoid an extra Ref call. + union { + absl::Status status_; + }; + + // data_ is active iff status_.ok()==true + struct Dummy {}; + union { + // When T is const, we need some non-const object we can cast to void* for + // the placement new. dummy_ is that object. + Dummy dummy_; + T data_; + }; + + void Clear() { + if (ok()) + data_.~T(); + } + + void EnsureOk() const { + if (!ok()) + Helper::Crash(status_); + } + + void EnsureNotOk() { + if (ok()) + Helper::HandleInvalidStatusCtorArg(&status_); + } + + // Construct the value (i.e. data_) through placement new with the passed + // argument. + template void MakeValue(Arg&& arg) { + StatusOr_Internal::PlacementNew(&dummy_, std::forward(arg)); + } + + // Construct the status (i.e. status_) through placement new with the passed + // argument. + template void MakeStatus(Args&&... args) { + StatusOr_Internal::PlacementNew(&status_, std::forward(args)...); + } +}; + +// Helper base class to allow implicitly deleted constructors and assignment +// operations in StatusOr. +// TraitsBase will explicitly delete what it can't support and StatusOr will +// inherit that behavior implicitly. +template struct TraitsBase { + TraitsBase() = default; + TraitsBase(const TraitsBase&) = default; + TraitsBase(TraitsBase&&) = default; + TraitsBase& operator=(const TraitsBase&) = default; + TraitsBase& operator=(TraitsBase&&) = default; +}; + +template <> struct TraitsBase { + TraitsBase() = default; + TraitsBase(const TraitsBase&) = delete; + TraitsBase(TraitsBase&&) = default; + TraitsBase& operator=(const TraitsBase&) = delete; + TraitsBase& operator=(TraitsBase&&) = default; +}; + +template <> struct TraitsBase { + TraitsBase() = default; + TraitsBase(const TraitsBase&) = delete; + TraitsBase(TraitsBase&&) = delete; + TraitsBase& operator=(const TraitsBase&) = delete; + TraitsBase& operator=(TraitsBase&&) = delete; +}; + +} // namespace StatusOr_Internal + +} // namespace Envoy diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 9e7cb3dcd9ca9..23a33bfaef5b8 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -273,3 +273,12 @@ envoy_cc_test( "//source/common/common:status_lib", ], ) + +envoy_cc_test( + name = "statusor_test", + srcs = ["statusor_test.cc"], + deps = [ + "//source/common/common:status_lib", + "//source/common/common:statusor_lib", + ], +) diff --git a/test/common/common/statusor_test.cc b/test/common/common/statusor_test.cc new file mode 100644 index 0000000000000..491d87ebdef67 --- /dev/null +++ b/test/common/common/statusor_test.cc @@ -0,0 +1,440 @@ +/** + * IMPORTANT: this file is a fork of the soon to be open-source absl::StatusOr class. + * When the absl::StatusOr lands this file will be trimmed to just Envoy specific use case. + */ + +/* Copyright 2017 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ + +// Unit tests for StatusOr + +#include +#include + +#include "common/common/status.h" +#include "common/common/statusor.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +class Base1 { +public: + virtual ~Base1() {} + int pad_; +}; + +class Base2 { +public: + virtual ~Base2() {} + int yetotherpad_; +}; + +class Derived : public Base1, public Base2 { +public: + ~Derived() override {} + int evenmorepad_; +}; + +class CopyNoAssign { +public: + explicit CopyNoAssign(int value) : foo_(value) {} + CopyNoAssign(const CopyNoAssign& other) : foo_(other.foo_) {} + int foo_; + +private: + const CopyNoAssign& operator=(const CopyNoAssign&); +}; + +class NoDefaultConstructor { +public: + explicit NoDefaultConstructor(int foo); +}; + +static_assert(!std::is_default_constructible(), + "Should not be default-constructible."); + +StatusOr> ReturnUniquePtr() { + // Uses implicit constructor from T&& + return std::unique_ptr(new int(0)); +} + +TEST(StatusOr, ElementType) { + static_assert(std::is_same::element_type, int>(), ""); + static_assert(std::is_same::element_type, char>(), ""); +} + +TEST(StatusOr, NullPointerStatusOr) { + // As a very special case, null-plain-pointer StatusOr used to be an + // error. Test that it no longer is. + StatusOr null_status(nullptr); + EXPECT_TRUE(null_status.ok()); + EXPECT_EQ(null_status.ValueOrDie(), nullptr); +} + +TEST(StatusOr, TestNoDefaultConstructorInitialization) { + // Explicitly initialize it with an error code. + StatusOr statusor(CodecProtocolError("")); + EXPECT_FALSE(statusor.ok()); + EXPECT_TRUE(IsCodecProtocolError(statusor.status())); + + // Default construction of StatusOr initializes it with an UNKNOWN error code. + StatusOr statusor2; + EXPECT_FALSE(statusor2.ok()); + EXPECT_DEATH(GetStatusCode(statusor2.status()), ""); +} + +TEST(StatusOr, TestMoveOnlyInitialization) { + StatusOr> thing(ReturnUniquePtr()); + ASSERT_TRUE(thing.ok()); + EXPECT_EQ(0, *thing.ValueOrDie()); + int* previous = thing.ValueOrDie().get(); + + thing = ReturnUniquePtr(); + EXPECT_TRUE(thing.ok()); + EXPECT_EQ(0, *thing.ValueOrDie()); + EXPECT_NE(previous, thing.ValueOrDie().get()); +} + +TEST(StatusOr, TestMoveOnlyStatusCtr) { + StatusOr> thing(CodecProtocolError("")); + ASSERT_FALSE(thing.ok()); +} + +TEST(StatusOr, TestMoveOnlyValueExtraction) { + StatusOr> thing(ReturnUniquePtr()); + ASSERT_TRUE(thing.ok()); + std::unique_ptr ptr = std::move(thing).ValueOrDie(); + EXPECT_EQ(0, *ptr); + + thing = std::move(ptr); + ptr = std::move(thing.ValueOrDie()); + EXPECT_EQ(0, *ptr); +} + +TEST(StatusOr, TestMoveOnlyConversion) { + StatusOr> const_thing(ReturnUniquePtr()); + EXPECT_TRUE(const_thing.ok()); + EXPECT_EQ(0, *const_thing.ValueOrDie()); + + // Test r-value converting assignment + const int* const_previous = const_thing.ValueOrDie().get(); + const_thing = ReturnUniquePtr(); + EXPECT_TRUE(const_thing.ok()); + EXPECT_EQ(0, *const_thing.ValueOrDie()); + EXPECT_NE(const_previous, const_thing.ValueOrDie().get()); +} + +TEST(StatusOr, TestMoveOnlyVector) { + // Sanity check that StatusOr works in vector. + std::vector>> vec; + vec.push_back(ReturnUniquePtr()); + vec.resize(2); + auto another_vec = std::move(vec); + EXPECT_EQ(0, *another_vec[0].ValueOrDie()); + EXPECT_DEATH(GetStatusCode(another_vec[1].status()), ""); +} + +TEST(StatusOr, TestMoveWithValuesAndErrors) { + StatusOr status_or(std::string(1000, '0')); + StatusOr value1(std::string(1000, '1')); + StatusOr value2(std::string(1000, '2')); + StatusOr error1(CodecProtocolError("error1")); + StatusOr error2(CodecProtocolError("error2")); + + ASSERT_TRUE(status_or.ok()); + EXPECT_EQ(std::string(1000, '0'), status_or.ValueOrDie()); + + // Overwrite the value in status_or with another value. + status_or = std::move(value1); + ASSERT_TRUE(status_or.ok()); + EXPECT_EQ(std::string(1000, '1'), status_or.ValueOrDie()); + + // Overwrite the value in status_or with an error. + status_or = std::move(error1); + ASSERT_FALSE(status_or.ok()); + EXPECT_EQ("error1", status_or.status().message()); + + // Overwrite the error in status_or with another error. + status_or = std::move(error2); + ASSERT_FALSE(status_or.ok()); + EXPECT_EQ("error2", status_or.status().message()); + + // Overwrite the error with a value. + status_or = std::move(value2); + ASSERT_TRUE(status_or.ok()); + EXPECT_EQ(std::string(1000, '2'), status_or.ValueOrDie()); +} + +TEST(StatusOr, TestCopyWithValuesAndErrors) { + StatusOr status_or(std::string(1000, '0')); + StatusOr value1(std::string(1000, '1')); + StatusOr value2(std::string(1000, '2')); + StatusOr error1(CodecProtocolError("error1")); + StatusOr error2(CodecProtocolError("error2")); + + ASSERT_TRUE(status_or.ok()); + EXPECT_EQ(std::string(1000, '0'), status_or.ValueOrDie()); + + // Overwrite the value in status_or with another value. + status_or = value1; + ASSERT_TRUE(status_or.ok()); + EXPECT_EQ(std::string(1000, '1'), status_or.ValueOrDie()); + + // Overwrite the value in status_or with an error. + status_or = error1; + ASSERT_FALSE(status_or.ok()); + EXPECT_EQ("error1", status_or.status().message()); + + // Overwrite the error in status_or with another error. + status_or = error2; + ASSERT_FALSE(status_or.ok()); + EXPECT_EQ("error2", status_or.status().message()); + + // Overwrite the error with a value. + status_or = value2; + ASSERT_TRUE(status_or.ok()); + EXPECT_EQ(std::string(1000, '2'), status_or.ValueOrDie()); + + // Verify original values unchanged. + EXPECT_EQ(std::string(1000, '1'), value1.ValueOrDie()); + EXPECT_EQ("error1", error1.status().message()); + EXPECT_EQ("error2", error2.status().message()); + EXPECT_EQ(std::string(1000, '2'), value2.ValueOrDie()); +} + +TEST(StatusOr, TestDefaultCtor) { + StatusOr thing; + EXPECT_FALSE(thing.ok()); + EXPECT_DEATH(GetStatusCode(thing.status()), ""); +} + +TEST(StatusOrDeathTest, TestDefaultCtorValue) { + StatusOr thing; + EXPECT_DEATH(thing.ValueOrDie(), ""); + + const StatusOr thing2; + EXPECT_DEATH(thing.ValueOrDie(), ""); +} + +TEST(StatusOr, TestStatusCtor) { + StatusOr thing(CodecProtocolError("")); + EXPECT_FALSE(thing.ok()); + EXPECT_TRUE(IsCodecProtocolError(thing.status())); +} + +TEST(StatusOr, TestValueCtor) { + const int kI = 4; + const StatusOr thing(kI); + EXPECT_TRUE(thing.ok()); + EXPECT_EQ(kI, thing.ValueOrDie()); +} + +TEST(StatusOr, TestCopyCtorStatusOk) { + const int kI = 4; + const StatusOr original(kI); + const StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); + EXPECT_EQ(original.ValueOrDie(), copy.ValueOrDie()); +} + +TEST(StatusOr, TestCopyCtorStatusNotOk) { + StatusOr original(CodecProtocolError("")); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); +} + +TEST(StatusOr, TestCopyCtorNonAssignable) { + const int kI = 4; + CopyNoAssign value(kI); + StatusOr original(value); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); + EXPECT_EQ(original.ValueOrDie().foo_, copy.ValueOrDie().foo_); +} + +TEST(StatusOr, TestCopyCtorStatusOKConverting) { + const int kI = 4; + StatusOr original(kI); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); + EXPECT_DOUBLE_EQ(original.ValueOrDie(), copy.ValueOrDie()); +} + +TEST(StatusOr, TestCopyCtorStatusNotOkConverting) { + StatusOr original(CodecProtocolError("")); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); +} + +TEST(StatusOr, TestAssignmentStatusOk) { + const int kI = 4; + StatusOr source(kI); + StatusOr target; + target = source; + EXPECT_EQ(target.status(), source.status()); + EXPECT_EQ(source.ValueOrDie(), target.ValueOrDie()); +} + +TEST(StatusOr, TestAssignmentStatusNotOk) { + StatusOr source(CodecProtocolError("")); + StatusOr target; + target = source; + EXPECT_EQ(target.status(), source.status()); +} + +TEST(StatusOr, TestStatus) { + StatusOr good(4); + EXPECT_TRUE(good.ok()); + StatusOr bad(CodecProtocolError("")); + EXPECT_FALSE(bad.ok()); + EXPECT_EQ(bad.status(), CodecProtocolError("")); +} + +TEST(StatusOr, TestValue) { + const int kI = 4; + StatusOr thing(kI); + EXPECT_EQ(kI, thing.ValueOrDie()); +} + +TEST(StatusOr, TestValueConst) { + const int kI = 4; + const StatusOr thing(kI); + EXPECT_EQ(kI, thing.ValueOrDie()); +} + +TEST(StatusOrDeathTest, TestValueNotOk) { + StatusOr thing(CodecProtocolError("cancelled")); + EXPECT_DEATH(thing.ValueOrDie(), ""); +} + +TEST(StatusOrDeathTest, TestValueNotOkConst) { + const StatusOr thing(CodecProtocolError("")); + EXPECT_DEATH(thing.ValueOrDie(), ""); +} + +TEST(StatusOr, TestPointerDefaultCtor) { + StatusOr thing; + EXPECT_FALSE(thing.ok()); + EXPECT_DEATH(GetStatusCode(thing.status()), ""); +} + +TEST(StatusOrDeathTest, TestPointerDefaultCtorValue) { + StatusOr thing; + EXPECT_DEATH(thing.ValueOrDie(), ""); +} + +TEST(StatusOr, TestPointerStatusCtor) { + StatusOr thing(CodecProtocolError("")); + EXPECT_FALSE(thing.ok()); + EXPECT_EQ(thing.status(), CodecProtocolError("")); +} + +TEST(StatusOr, TestPointerValueCtor) { + const int kI = 4; + StatusOr thing(&kI); + EXPECT_TRUE(thing.ok()); + EXPECT_EQ(&kI, thing.ValueOrDie()); +} + +TEST(StatusOr, TestPointerCopyCtorStatusOk) { + const int kI = 0; + StatusOr original(&kI); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); + EXPECT_EQ(original.ValueOrDie(), copy.ValueOrDie()); +} + +TEST(StatusOr, TestPointerCopyCtorStatusNotOk) { + StatusOr original(CodecProtocolError("")); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); +} + +TEST(StatusOr, TestPointerCopyCtorStatusOKConverting) { + Derived derived; + StatusOr original(&derived); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); + EXPECT_EQ(static_cast(original.ValueOrDie()), copy.ValueOrDie()); +} + +TEST(StatusOr, TestPointerCopyCtorStatusNotOkConverting) { + StatusOr original(CodecProtocolError("")); + StatusOr copy(original); + EXPECT_EQ(copy.status(), original.status()); +} + +TEST(StatusOr, TestPointerAssignmentStatusOk) { + const int kI = 0; + StatusOr source(&kI); + StatusOr target; + target = source; + EXPECT_EQ(target.status(), source.status()); + EXPECT_EQ(source.ValueOrDie(), target.ValueOrDie()); +} + +TEST(StatusOr, TestPointerAssignmentStatusNotOk) { + StatusOr source(CodecProtocolError("")); + StatusOr target; + target = source; + EXPECT_EQ(target.status(), source.status()); +} + +TEST(StatusOr, TestPointerStatus) { + const int kI = 0; + StatusOr good(&kI); + EXPECT_TRUE(good.ok()); + StatusOr bad(CodecProtocolError("")); + EXPECT_EQ(bad.status(), CodecProtocolError("")); +} + +TEST(StatusOr, TestPointerValue) { + const int kI = 0; + StatusOr thing(&kI); + EXPECT_EQ(&kI, thing.ValueOrDie()); +} + +TEST(StatusOr, TestPointerValueConst) { + const int kI = 0; + const StatusOr thing(&kI); + EXPECT_EQ(&kI, thing.ValueOrDie()); +} + +// NOTE(tucker): StatusOr does not support this kind +// of resize op. +// TEST(StatusOr, StatusOrVectorOfUniquePointerCanResize) { +// using EvilType = std::vector>; +// static_assert(std::is_copy_constructible::value, ""); +// std::vector> v(5); +// v.reserve(v.capacity() + 10); +// } + +TEST(StatusOrDeathTest, TestPointerValueNotOk) { + StatusOr thing(CodecProtocolError("cancelled")); + EXPECT_DEATH(thing.ValueOrDie(), ""); +} + +TEST(StatusOrDeathTest, TestPointerValueNotOkConst) { + const StatusOr thing(CodecProtocolError("cancelled")); + EXPECT_DEATH(thing.ValueOrDie(), ""); +} + +// Benchmarks were removed as we not intend to change forked code. + +} // namespace +} // namespace Envoy diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 7cba2f0a565d6..9ce1664c79aa3 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -1152,3 +1152,4 @@ zlib OBQ SemVer SCM +LLC From 552f8823175ea889cf61d5bf4e4c37cddd5b45f0 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Wed, 8 Apr 2020 17:25:59 -0400 Subject: [PATCH 05/11] Create StatusOr fork in the absl namespace Signed-off-by: Yan Avlasov --- source/common/common/BUILD | 15 -- test/common/common/BUILD | 9 - third_party/statusor/BUILD | 33 ++++ .../statusor}/statusor.cc | 13 +- .../statusor}/statusor.h | 76 +++----- .../statusor}/statusor_internals.h | 12 +- .../statusor}/statusor_test.cc | 163 +++++++++--------- 7 files changed, 155 insertions(+), 166 deletions(-) create mode 100644 third_party/statusor/BUILD rename {source/common/common => third_party/statusor}/statusor.cc (79%) rename {source/common/common => third_party/statusor}/statusor.h (83%) rename {source/common/common => third_party/statusor}/statusor_internals.h (96%) rename {test/common/common => third_party/statusor}/statusor_test.cc (69%) diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 43e74a1cfd674..264aa8314e9b7 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -441,18 +441,3 @@ envoy_cc_library( "//include/envoy/http:codes_interface", ], ) - -envoy_cc_library( - name = "statusor_lib", - srcs = [ - "statusor.cc", - ], - hdrs = [ - "statusor.h", - "statusor_internals.h", - ], - external_deps = [ - "abseil_status", - ], - deps = [":assert_lib"], -) diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 23a33bfaef5b8..9e7cb3dcd9ca9 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -273,12 +273,3 @@ envoy_cc_test( "//source/common/common:status_lib", ], ) - -envoy_cc_test( - name = "statusor_test", - srcs = ["statusor_test.cc"], - deps = [ - "//source/common/common:status_lib", - "//source/common/common:statusor_lib", - ], -) diff --git a/third_party/statusor/BUILD b/third_party/statusor/BUILD new file mode 100644 index 0000000000000..59f2262607425 --- /dev/null +++ b/third_party/statusor/BUILD @@ -0,0 +1,33 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_cc_test", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "statusor_lib", + srcs = [ + "statusor.cc", + ], + hdrs = [ + "statusor.h", + "statusor_internals.h", + ], + external_deps = [ + "abseil_status", + ], + deps = ["//source/common/common:assert_lib"], +) + +envoy_cc_test( + name = "statusor_test", + srcs = ["statusor_test.cc"], + deps = [ + "//third_party/statusor:statusor_lib", + ], +) diff --git a/source/common/common/statusor.cc b/third_party/statusor/statusor.cc similarity index 79% rename from source/common/common/statusor.cc rename to third_party/statusor/statusor.cc index bc9782aa7594b..20105404217ad 100644 --- a/source/common/common/statusor.cc +++ b/third_party/statusor/statusor.cc @@ -1,3 +1,8 @@ +/** + * IMPORTANT: this file is a fork of the soon to be open-source absl::StatusOr class. + * When the absl::StatusOr lands this file will be removed. + */ + /* * Copyright 2019 Google LLC * @@ -14,15 +19,15 @@ * limitations under the License. */ -#include "common/common/statusor.h" +#include "third_party/statusor/statusor.h" #include #include "common/common/assert.h" -namespace Envoy { +namespace absl { -namespace StatusOr_Internal { +namespace internal_statusor { void Helper::HandleInvalidStatusCtorArg(absl::Status* status) { const char* kMessage = "An OK status is not a valid constructor argument to StatusOr"; @@ -33,6 +38,6 @@ void Helper::HandleInvalidStatusCtorArg(absl::Status* status) { void Helper::Crash(const absl::Status&) { abort(); } -} // namespace StatusOr_Internal +} // namespace internal_statusor } // namespace Envoy diff --git a/source/common/common/statusor.h b/third_party/statusor/statusor.h similarity index 83% rename from source/common/common/statusor.h rename to third_party/statusor/statusor.h index 121b1274e665f..d8a7d48329e0b 100644 --- a/source/common/common/statusor.h +++ b/third_party/statusor/statusor.h @@ -1,32 +1,6 @@ /** * IMPORTANT: this file is a fork of the soon to be open-source absl::StatusOr class. - * When the absl::StatusOr lands this file will just define Envoy::StatusOr as an alias - * of absl::StatusOr. - * - * IMPORTANT: StatusOr default constructor must not be used as it does not fit intro any - * Envoy's use cases. The error extracting functions in the common/common/status.h will - * RELEASE_ASSERT on default initialized StatusOr. - * Usage example: - * - * Envoy::StatusOr Foo() { - * ... - * if (codec_error) { - * return CodecProtocolError("Invalid protocol"); - * } - * return 123456; - * } - * - * void Bar() { - * auto status_or = Foo(); - * if (status_or.ok()) { - * int result = status_or.ValueOrDie(); - * ... - * } else { - * ASSERT(IsCodecProtocolError(status_or.status())); - * ENVOY_LOG(debug, "Codec error encountered: {}", status_or.status().message()); - * } - * } - + * When the absl::StatusOr lands this file will be removed. */ /* @@ -66,7 +40,7 @@ // Example that is guaranteed crash if the result holds no value: // // StatusOr result = DoBigCalculationThatCouldFail(); -// const Foo& foo = result.ValueOrDie(); +// const Foo& foo = result.value(); // foo.DoSomethingCool(); // // Example usage of a StatusOr>: @@ -96,23 +70,23 @@ #include #include -#include "common/common/statusor_internals.h" +#include "third_party/statusor/statusor_internals.h" #include "absl/base/attributes.h" #include "absl/base/macros.h" -namespace Envoy { +namespace absl { // Returned StatusOr objects may not be ignored. template class ABSL_MUST_USE_RESULT StatusOr; template -class StatusOr : private StatusOr_Internal::StatusOrData, - private StatusOr_Internal::TraitsBase::value, +class StatusOr : private internal_statusor::StatusOrData, + private internal_statusor::TraitsBase::value, std::is_move_constructible::value> { template friend class StatusOr; - typedef StatusOr_Internal::StatusOrData Base; + typedef internal_statusor::StatusOrData Base; public: using element_type = T; @@ -145,7 +119,7 @@ class StatusOr : private StatusOr_Internal::StatusOrData, // Constructs a new StatusOr with the given value. After calling this // constructor, this->ok() will be true and the contained value may be - // retrieved with ValueOrDie(), operator*(), or operator->(). + // retrieved with value(), operator*(), or operator->(). // // NOTE: Not explicit - we want to use StatusOr as a return type // so it is convenient and sensible to be able to do 'return T()' @@ -155,7 +129,7 @@ class StatusOr : private StatusOr_Internal::StatusOrData, StatusOr(const T& value); // Constructs a new StatusOr with the given non-ok status. After calling this - // constructor, this->ok() will be false and calls to ValueOrDie() will + // constructor, this->ok() will be false and calls to value() will // CHECK-fail. // // NOTE: Not explicit - we want to use StatusOr as a return @@ -191,36 +165,36 @@ class StatusOr : private StatusOr_Internal::StatusOrData, // Returns a reference to our current value, or ASSERT-fails if !this->ok(). If // you have already checked the status using this->ok() or operator bool(), // then you probably want to use operator*() or operator->() to access the - // current value instead of ValueOrDie(). + // current value instead of value(). // // Note: for value types that are cheap to copy, prefer simple code: // - // T value = status_or.ValueOrDie(); + // T value = status_or.value(); // // Otherwise, if the value type is expensive to copy, but can be left // in the StatusOr, simply assign to a reference: // - // T& value = status_or.ValueOrDie(); // or `const T&` + // T& value = status_or.value(); // or `const T&` // // Otherwise, if the value type supports an efficient move, it can be // used as follows: // - // T value = std::move(status_or).ValueOrDie(); + // T value = std::move(status_or).value(); // // The std::move on status_or instead of on the whole expression enables // warnings about possible uses of the status_or object after the move. - const T& ValueOrDie() const&; - T& ValueOrDie() &; - const T&& ValueOrDie() const&&; - T&& ValueOrDie() &&; + const T& value() const&; + T& value() &; + const T&& value() const&&; + T&& value() &&; // Returns a reference to the current value. // // REQUIRES: this->ok() == true, otherwise the behavior is undefined. // // Use this->ok() or `operator bool()` to verify that there is a current - // value. Alternatively, see ValueOrDie() for a similar API that guarantees + // value. Alternatively, see value() for a similar API that guarantees // ASSERT-failing if there is no current value. const T& operator*() const&; T& operator*() &; @@ -280,7 +254,7 @@ template template inline StatusOr& StatusOr::operator=(const StatusOr& other) { if (other.ok()) - this->Assign(other.ValueOrDie()); + this->Assign(other.value()); else this->Assign(other.status()); return *this; @@ -295,7 +269,7 @@ template template inline StatusOr& StatusOr::operator=(StatusOr&& other) { if (other.ok()) { - this->Assign(std::move(other).ValueOrDie()); + this->Assign(std::move(other).value()); } else { this->Assign(std::move(other).status()); } @@ -307,22 +281,22 @@ template absl::Status StatusOr::status() && { return ok() ? absl::OkStatus() : std::move(this->status_); } -template const T& StatusOr::ValueOrDie() const& { +template const T& StatusOr::value() const& { this->EnsureOk(); return this->data_; } -template T& StatusOr::ValueOrDie() & { +template T& StatusOr::value() & { this->EnsureOk(); return this->data_; } -template const T&& StatusOr::ValueOrDie() const&& { +template const T&& StatusOr::value() const&& { this->EnsureOk(); return std::move(this->data_); } -template T&& StatusOr::ValueOrDie() && { +template T&& StatusOr::value() && { this->EnsureOk(); return std::move(this->data_); } @@ -375,4 +349,4 @@ template void StatusOr::IgnoreError() const { // no-op } -} // namespace Envoy +} // namespace absl diff --git a/source/common/common/statusor_internals.h b/third_party/statusor/statusor_internals.h similarity index 96% rename from source/common/common/statusor_internals.h rename to third_party/statusor/statusor_internals.h index 8c7205322bddb..ffe3f91af2c75 100644 --- a/source/common/common/statusor_internals.h +++ b/third_party/statusor/statusor_internals.h @@ -27,9 +27,9 @@ #include "absl/meta/type_traits.h" #include "absl/status/status.h" -namespace Envoy { +namespace absl { -namespace StatusOr_Internal { +namespace internal_statusor { class Helper { public: @@ -201,13 +201,13 @@ template class StatusOrData { // Construct the value (i.e. data_) through placement new with the passed // argument. template void MakeValue(Arg&& arg) { - StatusOr_Internal::PlacementNew(&dummy_, std::forward(arg)); + internal_statusor::PlacementNew(&dummy_, std::forward(arg)); } // Construct the status (i.e. status_) through placement new with the passed // argument. template void MakeStatus(Args&&... args) { - StatusOr_Internal::PlacementNew(&status_, std::forward(args)...); + internal_statusor::PlacementNew(&status_, std::forward(args)...); } }; @@ -239,6 +239,6 @@ template <> struct TraitsBase { TraitsBase& operator=(TraitsBase&&) = delete; }; -} // namespace StatusOr_Internal +} // namespace internal_statusor -} // namespace Envoy +} // namespace absl diff --git a/test/common/common/statusor_test.cc b/third_party/statusor/statusor_test.cc similarity index 69% rename from test/common/common/statusor_test.cc rename to third_party/statusor/statusor_test.cc index 491d87ebdef67..b627c518476e1 100644 --- a/test/common/common/statusor_test.cc +++ b/third_party/statusor/statusor_test.cc @@ -1,6 +1,6 @@ /** * IMPORTANT: this file is a fork of the soon to be open-source absl::StatusOr class. - * When the absl::StatusOr lands this file will be trimmed to just Envoy specific use case. + * When the absl::StatusOr lands this file will be removed. */ /* Copyright 2017 The TensorFlow Authors. All Rights Reserved. @@ -23,45 +23,45 @@ limitations under the License. #include #include -#include "common/common/status.h" -#include "common/common/statusor.h" +#include "third_party/statusor/statusor.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -namespace Envoy { +namespace absl { namespace { + class Base1 { -public: + public: virtual ~Base1() {} int pad_; }; class Base2 { -public: + public: virtual ~Base2() {} int yetotherpad_; }; class Derived : public Base1, public Base2 { -public: + public: ~Derived() override {} int evenmorepad_; }; class CopyNoAssign { -public: + public: explicit CopyNoAssign(int value) : foo_(value) {} CopyNoAssign(const CopyNoAssign& other) : foo_(other.foo_) {} int foo_; -private: + private: const CopyNoAssign& operator=(const CopyNoAssign&); }; class NoDefaultConstructor { -public: + public: explicit NoDefaultConstructor(int foo); }; @@ -83,60 +83,60 @@ TEST(StatusOr, NullPointerStatusOr) { // error. Test that it no longer is. StatusOr null_status(nullptr); EXPECT_TRUE(null_status.ok()); - EXPECT_EQ(null_status.ValueOrDie(), nullptr); + EXPECT_EQ(null_status.value(), nullptr); } TEST(StatusOr, TestNoDefaultConstructorInitialization) { // Explicitly initialize it with an error code. - StatusOr statusor(CodecProtocolError("")); + StatusOr statusor(absl::CancelledError("")); EXPECT_FALSE(statusor.ok()); - EXPECT_TRUE(IsCodecProtocolError(statusor.status())); + EXPECT_EQ(statusor.status().code(), absl::StatusCode::kCancelled); // Default construction of StatusOr initializes it with an UNKNOWN error code. StatusOr statusor2; EXPECT_FALSE(statusor2.ok()); - EXPECT_DEATH(GetStatusCode(statusor2.status()), ""); + EXPECT_EQ(statusor2.status().code(), absl::StatusCode::kUnknown); } TEST(StatusOr, TestMoveOnlyInitialization) { StatusOr> thing(ReturnUniquePtr()); ASSERT_TRUE(thing.ok()); - EXPECT_EQ(0, *thing.ValueOrDie()); - int* previous = thing.ValueOrDie().get(); + EXPECT_EQ(0, *thing.value()); + int* previous = thing.value().get(); thing = ReturnUniquePtr(); EXPECT_TRUE(thing.ok()); - EXPECT_EQ(0, *thing.ValueOrDie()); - EXPECT_NE(previous, thing.ValueOrDie().get()); + EXPECT_EQ(0, *thing.value()); + EXPECT_NE(previous, thing.value().get()); } TEST(StatusOr, TestMoveOnlyStatusCtr) { - StatusOr> thing(CodecProtocolError("")); + StatusOr> thing(absl::CancelledError("")); ASSERT_FALSE(thing.ok()); } TEST(StatusOr, TestMoveOnlyValueExtraction) { StatusOr> thing(ReturnUniquePtr()); ASSERT_TRUE(thing.ok()); - std::unique_ptr ptr = std::move(thing).ValueOrDie(); + std::unique_ptr ptr = std::move(thing).value(); EXPECT_EQ(0, *ptr); thing = std::move(ptr); - ptr = std::move(thing.ValueOrDie()); + ptr = std::move(thing.value()); EXPECT_EQ(0, *ptr); } TEST(StatusOr, TestMoveOnlyConversion) { StatusOr> const_thing(ReturnUniquePtr()); EXPECT_TRUE(const_thing.ok()); - EXPECT_EQ(0, *const_thing.ValueOrDie()); + EXPECT_EQ(0, *const_thing.value()); - // Test r-value converting assignment - const int* const_previous = const_thing.ValueOrDie().get(); + // Test rvalue converting assignment + const int* const_previous = const_thing.value().get(); const_thing = ReturnUniquePtr(); EXPECT_TRUE(const_thing.ok()); - EXPECT_EQ(0, *const_thing.ValueOrDie()); - EXPECT_NE(const_previous, const_thing.ValueOrDie().get()); + EXPECT_EQ(0, *const_thing.value()); + EXPECT_NE(const_previous, const_thing.value().get()); } TEST(StatusOr, TestMoveOnlyVector) { @@ -145,24 +145,24 @@ TEST(StatusOr, TestMoveOnlyVector) { vec.push_back(ReturnUniquePtr()); vec.resize(2); auto another_vec = std::move(vec); - EXPECT_EQ(0, *another_vec[0].ValueOrDie()); - EXPECT_DEATH(GetStatusCode(another_vec[1].status()), ""); + EXPECT_EQ(0, *another_vec[0].value()); + EXPECT_EQ(absl::StatusCode::kUnknown, another_vec[1].status().code()); } TEST(StatusOr, TestMoveWithValuesAndErrors) { StatusOr status_or(std::string(1000, '0')); StatusOr value1(std::string(1000, '1')); StatusOr value2(std::string(1000, '2')); - StatusOr error1(CodecProtocolError("error1")); - StatusOr error2(CodecProtocolError("error2")); + StatusOr error1(Status(absl::StatusCode::kUnknown, "error1")); + StatusOr error2(Status(absl::StatusCode::kUnknown, "error2")); ASSERT_TRUE(status_or.ok()); - EXPECT_EQ(std::string(1000, '0'), status_or.ValueOrDie()); + EXPECT_EQ(std::string(1000, '0'), status_or.value()); // Overwrite the value in status_or with another value. status_or = std::move(value1); ASSERT_TRUE(status_or.ok()); - EXPECT_EQ(std::string(1000, '1'), status_or.ValueOrDie()); + EXPECT_EQ(std::string(1000, '1'), status_or.value()); // Overwrite the value in status_or with an error. status_or = std::move(error1); @@ -177,23 +177,23 @@ TEST(StatusOr, TestMoveWithValuesAndErrors) { // Overwrite the error with a value. status_or = std::move(value2); ASSERT_TRUE(status_or.ok()); - EXPECT_EQ(std::string(1000, '2'), status_or.ValueOrDie()); + EXPECT_EQ(std::string(1000, '2'), status_or.value()); } TEST(StatusOr, TestCopyWithValuesAndErrors) { StatusOr status_or(std::string(1000, '0')); StatusOr value1(std::string(1000, '1')); StatusOr value2(std::string(1000, '2')); - StatusOr error1(CodecProtocolError("error1")); - StatusOr error2(CodecProtocolError("error2")); + StatusOr error1(Status(absl::StatusCode::kUnknown, "error1")); + StatusOr error2(Status(absl::StatusCode::kUnknown, "error2")); ASSERT_TRUE(status_or.ok()); - EXPECT_EQ(std::string(1000, '0'), status_or.ValueOrDie()); + EXPECT_EQ(std::string(1000, '0'), status_or.value()); // Overwrite the value in status_or with another value. status_or = value1; ASSERT_TRUE(status_or.ok()); - EXPECT_EQ(std::string(1000, '1'), status_or.ValueOrDie()); + EXPECT_EQ(std::string(1000, '1'), status_or.value()); // Overwrite the value in status_or with an error. status_or = error1; @@ -208,40 +208,40 @@ TEST(StatusOr, TestCopyWithValuesAndErrors) { // Overwrite the error with a value. status_or = value2; ASSERT_TRUE(status_or.ok()); - EXPECT_EQ(std::string(1000, '2'), status_or.ValueOrDie()); + EXPECT_EQ(std::string(1000, '2'), status_or.value()); // Verify original values unchanged. - EXPECT_EQ(std::string(1000, '1'), value1.ValueOrDie()); + EXPECT_EQ(std::string(1000, '1'), value1.value()); EXPECT_EQ("error1", error1.status().message()); EXPECT_EQ("error2", error2.status().message()); - EXPECT_EQ(std::string(1000, '2'), value2.ValueOrDie()); + EXPECT_EQ(std::string(1000, '2'), value2.value()); } TEST(StatusOr, TestDefaultCtor) { StatusOr thing; EXPECT_FALSE(thing.ok()); - EXPECT_DEATH(GetStatusCode(thing.status()), ""); + EXPECT_EQ(thing.status().code(), absl::StatusCode::kUnknown); } TEST(StatusOrDeathTest, TestDefaultCtorValue) { StatusOr thing; - EXPECT_DEATH(thing.ValueOrDie(), ""); + EXPECT_DEATH(thing.value(), ""); const StatusOr thing2; - EXPECT_DEATH(thing.ValueOrDie(), ""); + EXPECT_DEATH(thing.value(), ""); } TEST(StatusOr, TestStatusCtor) { - StatusOr thing(CodecProtocolError("")); + StatusOr thing(Status(absl::StatusCode::kCancelled, "")); EXPECT_FALSE(thing.ok()); - EXPECT_TRUE(IsCodecProtocolError(thing.status())); + EXPECT_EQ(thing.status().code(), absl::StatusCode::kCancelled); } TEST(StatusOr, TestValueCtor) { const int kI = 4; const StatusOr thing(kI); EXPECT_TRUE(thing.ok()); - EXPECT_EQ(kI, thing.ValueOrDie()); + EXPECT_EQ(kI, thing.value()); } TEST(StatusOr, TestCopyCtorStatusOk) { @@ -249,11 +249,11 @@ TEST(StatusOr, TestCopyCtorStatusOk) { const StatusOr original(kI); const StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); - EXPECT_EQ(original.ValueOrDie(), copy.ValueOrDie()); + EXPECT_EQ(original.value(), copy.value()); } TEST(StatusOr, TestCopyCtorStatusNotOk) { - StatusOr original(CodecProtocolError("")); + StatusOr original(Status(absl::StatusCode::kCancelled, "")); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); } @@ -264,7 +264,7 @@ TEST(StatusOr, TestCopyCtorNonAssignable) { StatusOr original(value); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); - EXPECT_EQ(original.ValueOrDie().foo_, copy.ValueOrDie().foo_); + EXPECT_EQ(original.value().foo_, copy.value().foo_); } TEST(StatusOr, TestCopyCtorStatusOKConverting) { @@ -272,11 +272,11 @@ TEST(StatusOr, TestCopyCtorStatusOKConverting) { StatusOr original(kI); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); - EXPECT_DOUBLE_EQ(original.ValueOrDie(), copy.ValueOrDie()); + EXPECT_DOUBLE_EQ(original.value(), copy.value()); } TEST(StatusOr, TestCopyCtorStatusNotOkConverting) { - StatusOr original(CodecProtocolError("")); + StatusOr original(Status(absl::StatusCode::kCancelled, "")); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); } @@ -287,11 +287,11 @@ TEST(StatusOr, TestAssignmentStatusOk) { StatusOr target; target = source; EXPECT_EQ(target.status(), source.status()); - EXPECT_EQ(source.ValueOrDie(), target.ValueOrDie()); + EXPECT_EQ(source.value(), target.value()); } TEST(StatusOr, TestAssignmentStatusNotOk) { - StatusOr source(CodecProtocolError("")); + StatusOr source(Status(absl::StatusCode::kCancelled, "")); StatusOr target; target = source; EXPECT_EQ(target.status(), source.status()); @@ -300,55 +300,55 @@ TEST(StatusOr, TestAssignmentStatusNotOk) { TEST(StatusOr, TestStatus) { StatusOr good(4); EXPECT_TRUE(good.ok()); - StatusOr bad(CodecProtocolError("")); + StatusOr bad(Status(absl::StatusCode::kCancelled, "")); EXPECT_FALSE(bad.ok()); - EXPECT_EQ(bad.status(), CodecProtocolError("")); + EXPECT_EQ(bad.status(), Status(absl::StatusCode::kCancelled, "")); } TEST(StatusOr, TestValue) { const int kI = 4; StatusOr thing(kI); - EXPECT_EQ(kI, thing.ValueOrDie()); + EXPECT_EQ(kI, thing.value()); } TEST(StatusOr, TestValueConst) { const int kI = 4; const StatusOr thing(kI); - EXPECT_EQ(kI, thing.ValueOrDie()); + EXPECT_EQ(kI, thing.value()); } TEST(StatusOrDeathTest, TestValueNotOk) { - StatusOr thing(CodecProtocolError("cancelled")); - EXPECT_DEATH(thing.ValueOrDie(), ""); + StatusOr thing(Status(absl::StatusCode::kCancelled, "cancelled")); + EXPECT_DEATH(thing.value(), ""); } TEST(StatusOrDeathTest, TestValueNotOkConst) { - const StatusOr thing(CodecProtocolError("")); - EXPECT_DEATH(thing.ValueOrDie(), ""); + const StatusOr thing(Status(absl::StatusCode::kUnknown, "")); + EXPECT_DEATH(thing.value(), ""); } TEST(StatusOr, TestPointerDefaultCtor) { StatusOr thing; EXPECT_FALSE(thing.ok()); - EXPECT_DEATH(GetStatusCode(thing.status()), ""); + EXPECT_EQ(thing.status().code(), absl::StatusCode::kUnknown); } TEST(StatusOrDeathTest, TestPointerDefaultCtorValue) { StatusOr thing; - EXPECT_DEATH(thing.ValueOrDie(), ""); + EXPECT_DEATH(thing.value(), ""); } TEST(StatusOr, TestPointerStatusCtor) { - StatusOr thing(CodecProtocolError("")); + StatusOr thing(Status(absl::StatusCode::kCancelled, "")); EXPECT_FALSE(thing.ok()); - EXPECT_EQ(thing.status(), CodecProtocolError("")); + EXPECT_EQ(thing.status(), Status(absl::StatusCode::kCancelled, "")); } TEST(StatusOr, TestPointerValueCtor) { const int kI = 4; StatusOr thing(&kI); EXPECT_TRUE(thing.ok()); - EXPECT_EQ(&kI, thing.ValueOrDie()); + EXPECT_EQ(&kI, thing.value()); } TEST(StatusOr, TestPointerCopyCtorStatusOk) { @@ -356,11 +356,11 @@ TEST(StatusOr, TestPointerCopyCtorStatusOk) { StatusOr original(&kI); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); - EXPECT_EQ(original.ValueOrDie(), copy.ValueOrDie()); + EXPECT_EQ(original.value(), copy.value()); } TEST(StatusOr, TestPointerCopyCtorStatusNotOk) { - StatusOr original(CodecProtocolError("")); + StatusOr original(Status(absl::StatusCode::kCancelled, "")); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); } @@ -370,11 +370,12 @@ TEST(StatusOr, TestPointerCopyCtorStatusOKConverting) { StatusOr original(&derived); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); - EXPECT_EQ(static_cast(original.ValueOrDie()), copy.ValueOrDie()); + EXPECT_EQ(static_cast(original.value()), + copy.value()); } TEST(StatusOr, TestPointerCopyCtorStatusNotOkConverting) { - StatusOr original(CodecProtocolError("")); + StatusOr original(Status(absl::StatusCode::kCancelled, "")); StatusOr copy(original); EXPECT_EQ(copy.status(), original.status()); } @@ -385,11 +386,11 @@ TEST(StatusOr, TestPointerAssignmentStatusOk) { StatusOr target; target = source; EXPECT_EQ(target.status(), source.status()); - EXPECT_EQ(source.ValueOrDie(), target.ValueOrDie()); + EXPECT_EQ(source.value(), target.value()); } TEST(StatusOr, TestPointerAssignmentStatusNotOk) { - StatusOr source(CodecProtocolError("")); + StatusOr source(Status(absl::StatusCode::kCancelled, "")); StatusOr target; target = source; EXPECT_EQ(target.status(), source.status()); @@ -399,20 +400,20 @@ TEST(StatusOr, TestPointerStatus) { const int kI = 0; StatusOr good(&kI); EXPECT_TRUE(good.ok()); - StatusOr bad(CodecProtocolError("")); - EXPECT_EQ(bad.status(), CodecProtocolError("")); + StatusOr bad(Status(absl::StatusCode::kCancelled, "")); + EXPECT_EQ(bad.status(), Status(absl::StatusCode::kCancelled, "")); } TEST(StatusOr, TestPointerValue) { const int kI = 0; StatusOr thing(&kI); - EXPECT_EQ(&kI, thing.ValueOrDie()); + EXPECT_EQ(&kI, thing.value()); } TEST(StatusOr, TestPointerValueConst) { const int kI = 0; const StatusOr thing(&kI); - EXPECT_EQ(&kI, thing.ValueOrDie()); + EXPECT_EQ(&kI, thing.value()); } // NOTE(tucker): StatusOr does not support this kind @@ -425,16 +426,16 @@ TEST(StatusOr, TestPointerValueConst) { // } TEST(StatusOrDeathTest, TestPointerValueNotOk) { - StatusOr thing(CodecProtocolError("cancelled")); - EXPECT_DEATH(thing.ValueOrDie(), ""); + StatusOr thing(Status(absl::StatusCode::kCancelled, "cancelled")); + EXPECT_DEATH(thing.value(), ""); } TEST(StatusOrDeathTest, TestPointerValueNotOkConst) { - const StatusOr thing(CodecProtocolError("cancelled")); - EXPECT_DEATH(thing.ValueOrDie(), ""); + const StatusOr thing(Status(absl::StatusCode::kCancelled, "cancelled")); + EXPECT_DEATH(thing.value(), ""); } // Benchmarks were removed as we not intend to change forked code. } // namespace -} // namespace Envoy +} // namespace absl From d857eefa906ef085e2985dee7da12fdfb13370e8 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Wed, 8 Apr 2020 20:03:15 -0400 Subject: [PATCH 06/11] Add Envoy::StatusOr alias and its unit tests Exclude third_party from check_format Signed-off-by: Yan Avlasov --- source/common/common/BUILD | 8 +++++ source/common/common/statusor.h | 42 ++++++++++++++++++++++++++ test/common/common/BUILD | 9 ++++++ test/common/common/statusor_test.cc | 23 ++++++++++++++ third_party/statusor/BUILD | 2 +- tools/code_format/check_format.py | 2 +- tools/spelling/spelling_dictionary.txt | 1 - 7 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 source/common/common/statusor.h create mode 100644 test/common/common/statusor_test.cc diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 264aa8314e9b7..6952c325308b5 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -441,3 +441,11 @@ envoy_cc_library( "//include/envoy/http:codes_interface", ], ) + +envoy_cc_library( + name = "statusor_lib", + hdrs = ["statusor.h"], + deps = [ + "//third_party/statusor:statusor_lib", + ], +) diff --git a/source/common/common/statusor.h b/source/common/common/statusor.h new file mode 100644 index 0000000000000..a4ea50cdcc3f0 --- /dev/null +++ b/source/common/common/statusor.h @@ -0,0 +1,42 @@ +#pragma once + +#include "third_party/statusor/statusor.h" + +/** + * Facility for returning either a valid value or an error in a form of Envoy::Status. + * + * IMPORTANT: StatusOr default constructor must not be used as it does not fit into any + * Envoy's use cases. The error extracting functions in the common/common/status.h will + * RELEASE_ASSERT on default initialized StatusOr. + * + * To return an error StatusOr object an error creating function from common/common/status.h must be + * used. + * TODO(yanavlasov): add clang-tidy or lint check to enforce this. + * + * Usage example: + * + * Envoy::StatusOr Foo() { + * ... + * if (codec_error) { + * return CodecProtocolError("Invalid protocol"); + * } + * return 123456; + * } + * + * void Bar() { + * auto status_or = Foo(); + * if (status_or.ok()) { + * int result = status_or.value(); + * ... + * } else { + * ASSERT(IsCodecProtocolError(status_or.status())); + * ENVOY_LOG(debug, "Codec error encountered: {}", status_or.status().message()); + * } + * } + */ + +namespace Envoy { + +using absl::StatusOr; + +} // namespace Envoy diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 9e7cb3dcd9ca9..23a33bfaef5b8 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -273,3 +273,12 @@ envoy_cc_test( "//source/common/common:status_lib", ], ) + +envoy_cc_test( + name = "statusor_test", + srcs = ["statusor_test.cc"], + deps = [ + "//source/common/common:status_lib", + "//source/common/common:statusor_lib", + ], +) diff --git a/test/common/common/statusor_test.cc b/test/common/common/statusor_test.cc new file mode 100644 index 0000000000000..a7b4993d8f7b0 --- /dev/null +++ b/test/common/common/statusor_test.cc @@ -0,0 +1,23 @@ +#include "common/common/status.h" +#include "common/common/statusor.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { + +TEST(StatusOr, Initialization) { + StatusOr statusor(PrematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired)); + EXPECT_FALSE(statusor.ok()); + EXPECT_TRUE(IsPrematureResponseError(statusor.status())); + EXPECT_EQ("foobar", statusor.status().message()); + EXPECT_EQ(Http::Code::ProxyAuthenticationRequired, + GetPrematureResponseHttpCode(statusor.status())); +} + +TEST(StatusOr, DefaultInitialization) { + StatusOr statusor; + EXPECT_DEATH(GetStatusCode(statusor.status()), ""); +} + +} // namespace Envoy diff --git a/third_party/statusor/BUILD b/third_party/statusor/BUILD index 59f2262607425..fe17659a53787 100644 --- a/third_party/statusor/BUILD +++ b/third_party/statusor/BUILD @@ -28,6 +28,6 @@ envoy_cc_test( name = "statusor_test", srcs = ["statusor_test.cc"], deps = [ - "//third_party/statusor:statusor_lib", + ":statusor_lib", ], ) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 416a8bd6eca37..d3392637bb7c6 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -18,7 +18,7 @@ EXCLUDED_PREFIXES = ("./generated/", "./thirdparty/", "./build", "./.git/", "./bazel-", "./.cache", "./source/extensions/extensions_build_config.bzl", "./bazel/toolchains/configs/", "./tools/testdata/check_format/", - "./tools/pyformat/") + "./tools/pyformat/", "./third_party/") SUFFIXES = ("BUILD", "WORKSPACE", ".bzl", ".cc", ".h", ".java", ".m", ".md", ".mm", ".proto", ".rst") DOCS_SUFFIX = (".md", ".rst") diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 9ce1664c79aa3..7cba2f0a565d6 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -1152,4 +1152,3 @@ zlib OBQ SemVer SCM -LLC From 185b00229e42c773e19c5e16a8400b849344bae9 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Wed, 8 Apr 2020 20:35:15 -0400 Subject: [PATCH 07/11] Exclude third_party from clang-tidy Signed-off-by: Yan Avlasov --- ci/run_clang_tidy.sh | 7 ++++++- source/common/common/statusor.h | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/ci/run_clang_tidy.sh b/ci/run_clang_tidy.sh index 56745372e0c03..b394ccfddd352 100755 --- a/ci/run_clang_tidy.sh +++ b/ci/run_clang_tidy.sh @@ -51,8 +51,13 @@ function exclude_chromium_url() { grep -v source/common/chromium_url/ } +# Exclude files in third_party which are temporary forks from other OSS projects. +function exclude_third_party() { + grep -v third_party/ +} + function filter_excludes() { - exclude_testdata | exclude_chromium_url | exclude_win32_impl + exclude_testdata | exclude_chromium_url | exclude_win32_impl | exclude_third_party } diff --git a/source/common/common/statusor.h b/source/common/common/statusor.h index a4ea50cdcc3f0..3fe21ccb9a7a8 100644 --- a/source/common/common/statusor.h +++ b/source/common/common/statusor.h @@ -37,6 +37,6 @@ namespace Envoy { -using absl::StatusOr; +using absl::StatusOr; // NOLINT: disable misc-unused-using-decls clang-tidy check } // namespace Envoy From 25329e5d4eaae9f364c853360f6449e650aa5080 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Thu, 9 Apr 2020 10:55:33 -0400 Subject: [PATCH 08/11] Address comments Signed-off-by: Yan Avlasov --- source/common/common/status.cc | 125 +++++++++++++--------------- source/common/common/status.h | 24 +++--- test/common/common/status_test.cc | 77 +++++++++-------- test/common/common/statusor_test.cc | 8 +- 4 files changed, 117 insertions(+), 117 deletions(-) diff --git a/source/common/common/status.cc b/source/common/common/status.cc index c6aa06dfe154a..726173ebd02f4 100644 --- a/source/common/common/status.cc +++ b/source/common/common/status.cc @@ -8,10 +8,10 @@ namespace { const char EnvoyPayloadUrl[] = {"Envoy"}; -absl::string_view StatusCodeToString(StatusCode code) { +absl::string_view statusCodeToString(StatusCode code) { switch (code) { - default: - RELEASE_ASSERT(false, "Unknown status code"); + case StatusCode::Ok: + return absl::OkStatus().ToString(); case StatusCode::CodecProtocolError: return "CodecProtocolError"; case StatusCode::BufferFloodError: @@ -23,104 +23,99 @@ absl::string_view StatusCodeToString(StatusCode code) { } } -absl::StatusCode ToAbslStatusCode(StatusCode code) { - switch (code) { - default: - RELEASE_ASSERT(false, "Unknown status code"); - case StatusCode::Ok: - return absl::StatusCode::kOk; - case StatusCode::CodecProtocolError: - return absl::StatusCode::kFailedPrecondition; - case StatusCode::BufferFloodError: - return absl::StatusCode::kResourceExhausted; - case StatusCode::PrematureResponseError: - return absl::StatusCode::kInternal; - case StatusCode::CodecClientError: - return absl::StatusCode::kUnimplemented; - } +struct EnvoyStatusPayload { + EnvoyStatusPayload(StatusCode status_code) : status_code_(status_code) {} + StatusCode status_code_; +}; + +struct PrematureResponsePayload : public EnvoyStatusPayload { + PrematureResponsePayload(Http::Code http_code) + : EnvoyStatusPayload(StatusCode::PrematureResponseError), http_code_(http_code) {} + Http::Code http_code_; +}; + +template void storePayload(absl::Status& status, const T& payload) { + status.SetPayload( + EnvoyPayloadUrl, + absl::Cord(absl::string_view(reinterpret_cast(&payload), sizeof(payload)))); } -StatusCode ToEnvoyStatusCode(absl::StatusCode code) { - switch (code) { - default: - RELEASE_ASSERT(false, "Unknown status code"); - case absl::StatusCode::kOk: - return StatusCode::Ok; - case absl::StatusCode::kFailedPrecondition: - return StatusCode::CodecProtocolError; - case absl::StatusCode::kResourceExhausted: - return StatusCode::BufferFloodError; - case absl::StatusCode::kInternal: - return StatusCode::PrematureResponseError; - case absl::StatusCode::kUnimplemented: - return StatusCode::CodecClientError; - } +template const T* getPayload(const absl::Status& status) { + auto payload = status.GetPayload(EnvoyPayloadUrl); + RELEASE_ASSERT(payload.has_value(), "Must have payload"); + auto data = payload.value().Flatten(); + RELEASE_ASSERT(data.length() >= sizeof(T), "Invalid payload length"); + return reinterpret_cast(data.data()); } } // namespace -std::string ToString(const Status& status) { +std::string toString(const Status& status) { if (status.ok()) { return status.ToString(); } std::string text; - if (!IsPrematureResponseError(status)) { - absl::StrAppend(&text, StatusCodeToString(ToEnvoyStatusCode(status.code())), ": ", - status.message()); + auto status_code = getStatusCode(status); + if (status_code != StatusCode::PrematureResponseError) { + absl::StrAppend(&text, statusCodeToString(status_code), ": ", status.message()); } else { - auto http_code = GetPrematureResponseHttpCode(status); + auto http_code = getPrematureResponseHttpCode(status); absl::StrAppend(&text, "PrematureResponseError: HTTP code: ", http_code, ": ", status.message()); } return text; } -Status CodecProtocolError(absl::string_view message) { - return absl::Status(ToAbslStatusCode(StatusCode::CodecProtocolError), message); +Status codecProtocolError(absl::string_view message) { + absl::Status status(absl::StatusCode::kInternal, message); + storePayload(status, EnvoyStatusPayload(StatusCode::CodecProtocolError)); + return status; } -Status BufferFloodError(absl::string_view message) { - return absl::Status(ToAbslStatusCode(StatusCode::BufferFloodError), message); +Status bufferFloodError(absl::string_view message) { + absl::Status status(absl::StatusCode::kInternal, message); + storePayload(status, EnvoyStatusPayload(StatusCode::BufferFloodError)); + return status; } -Status PrematureResponseError(absl::string_view message, Http::Code http_code) { - absl::Status status(ToAbslStatusCode(StatusCode::PrematureResponseError), message); - status.SetPayload( - EnvoyPayloadUrl, - absl::Cord(absl::string_view(reinterpret_cast(&http_code), sizeof(http_code)))); +Status prematureResponseError(absl::string_view message, Http::Code http_code) { + absl::Status status(absl::StatusCode::kInternal, message); + storePayload(status, PrematureResponsePayload(http_code)); return status; } -Status CodecClientError(absl::string_view message) { - return absl::Status(ToAbslStatusCode(StatusCode::CodecClientError), message); +Status codecClientError(absl::string_view message) { + absl::Status status(absl::StatusCode::kInternal, message); + storePayload(status, EnvoyStatusPayload(StatusCode::CodecClientError)); + return status; } // Methods for checking and extracting error information -StatusCode GetStatusCode(const Status& status) { return ToEnvoyStatusCode(status.code()); } +StatusCode getStatusCode(const Status& status) { + return status.ok() ? StatusCode::Ok : getPayload(status)->status_code_; +} -bool IsCodecProtocolError(const Status& status) { - return ToEnvoyStatusCode(status.code()) == StatusCode::CodecProtocolError; +bool isCodecProtocolError(const Status& status) { + return getStatusCode(status) == StatusCode::CodecProtocolError; } -bool IsBufferFloodError(const Status& status) { - return ToEnvoyStatusCode(status.code()) == StatusCode::BufferFloodError; +bool isBufferFloodError(const Status& status) { + return getStatusCode(status) == StatusCode::BufferFloodError; } -bool IsPrematureResponseError(const Status& status) { - return ToEnvoyStatusCode(status.code()) == StatusCode::PrematureResponseError; +bool isPrematureResponseError(const Status& status) { + return getStatusCode(status) == StatusCode::PrematureResponseError; } -Http::Code GetPrematureResponseHttpCode(const Status& status) { - RELEASE_ASSERT(IsPrematureResponseError(status), "Must be PrematureResponseError"); - auto payload = status.GetPayload(EnvoyPayloadUrl); - RELEASE_ASSERT(payload.has_value(), "Must have payload"); - auto data = payload.value().Flatten(); - RELEASE_ASSERT(data.length() == sizeof(Http::Code), "Invalid payload length"); - return *reinterpret_cast(data.data()); +Http::Code getPrematureResponseHttpCode(const Status& status) { + const auto* payload = getPayload(status); + RELEASE_ASSERT(payload->status_code_ == StatusCode::PrematureResponseError, + "Must be PrematureResponseError"); + return payload->http_code_; } -bool IsCodecClientError(const Status& status) { - return ToEnvoyStatusCode(status.code()) == StatusCode::CodecClientError; +bool isCodecClientError(const Status& status) { + return getStatusCode(status) == StatusCode::CodecClientError; } } // namespace Envoy diff --git a/source/common/common/status.h b/source/common/common/status.h index 80aaa2a4c02d6..2b2f89f16057c 100644 --- a/source/common/common/status.h +++ b/source/common/common/status.h @@ -55,40 +55,40 @@ enum class StatusCode : int { using Status = absl::Status; -inline Status OkStatus() { return absl::OkStatus(); } +inline Status okStatus() { return absl::OkStatus(); } /** * Returns the combination of the error code name, message and any additional error attributes. */ -std::string ToString(const Status& status); +std::string toString(const Status& status); /** * Functions for creating error values. The error code of the returned status object matches the * name of the function. */ -Status CodecProtocolError(absl::string_view message); -Status BufferFloodError(absl::string_view message); -Status PrematureResponseError(absl::string_view message, Http::Code http_code); -Status CodecClientError(absl::string_view message); +Status codecProtocolError(absl::string_view message); +Status bufferFloodError(absl::string_view message); +Status prematureResponseError(absl::string_view message, Http::Code http_code); +Status codecClientError(absl::string_view message); /** * Returns Envoy::StatusCode of the given status object. * If the status object does not contain valid Envoy::Status value the function will RELEASE_ASSERT. */ -StatusCode GetStatusCode(const Status& status); +StatusCode getStatusCode(const Status& status); /** * Returns true if the given status matches error code implied by the name of the function. */ -ABSL_MUST_USE_RESULT bool IsCodecProtocolError(const Status& status); -ABSL_MUST_USE_RESULT bool IsBufferFloodError(const Status& status); -ABSL_MUST_USE_RESULT bool IsPrematureResponseError(const Status& status); -ABSL_MUST_USE_RESULT bool IsCodecClientError(const Status& status); +ABSL_MUST_USE_RESULT bool isCodecProtocolError(const Status& status); +ABSL_MUST_USE_RESULT bool isBufferFloodError(const Status& status); +ABSL_MUST_USE_RESULT bool isPrematureResponseError(const Status& status); +ABSL_MUST_USE_RESULT bool isCodecClientError(const Status& status); /** * Returns Http::Code value of the PrematureResponseError status. * IsPrematureResponseError(status) must be true which is checked by RELEASE_ASSERT. */ -ABSL_MUST_USE_RESULT Http::Code GetPrematureResponseHttpCode(const Status& status); +Http::Code getPrematureResponseHttpCode(const Status& status); } // namespace Envoy diff --git a/test/common/common/status_test.cc b/test/common/common/status_test.cc index 15bc0860873f1..c25641a0fbaec 100644 --- a/test/common/common/status_test.cc +++ b/test/common/common/status_test.cc @@ -6,64 +6,69 @@ namespace Envoy { TEST(Status, Ok) { - auto status = OkStatus(); + auto status = okStatus(); EXPECT_TRUE(status.ok()); EXPECT_TRUE(status.message().empty()); - EXPECT_EQ("OK", ToString(status)); - EXPECT_EQ(StatusCode::Ok, GetStatusCode(status)); - EXPECT_FALSE(IsCodecProtocolError(status)); - EXPECT_FALSE(IsBufferFloodError(status)); - EXPECT_FALSE(IsPrematureResponseError(status)); - EXPECT_FALSE(IsCodecClientError(status)); + EXPECT_EQ("OK", toString(status)); + EXPECT_EQ(StatusCode::Ok, getStatusCode(status)); + EXPECT_FALSE(isCodecProtocolError(status)); + EXPECT_FALSE(isBufferFloodError(status)); + EXPECT_FALSE(isPrematureResponseError(status)); + EXPECT_FALSE(isCodecClientError(status)); } TEST(Status, CodecProtocolError) { - auto status = CodecProtocolError("foobar"); + auto status = codecProtocolError("foobar"); EXPECT_FALSE(status.ok()); EXPECT_EQ("foobar", status.message()); - EXPECT_EQ("CodecProtocolError: foobar", ToString(status)); - EXPECT_EQ(StatusCode::CodecProtocolError, GetStatusCode(status)); - EXPECT_TRUE(IsCodecProtocolError(status)); - EXPECT_FALSE(IsBufferFloodError(status)); - EXPECT_FALSE(IsPrematureResponseError(status)); - EXPECT_FALSE(IsCodecClientError(status)); + EXPECT_EQ("CodecProtocolError: foobar", toString(status)); + EXPECT_EQ(StatusCode::CodecProtocolError, getStatusCode(status)); + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_FALSE(isBufferFloodError(status)); + EXPECT_FALSE(isPrematureResponseError(status)); + EXPECT_FALSE(isCodecClientError(status)); } TEST(Status, BufferFloodError) { - auto status = BufferFloodError("foobar"); + auto status = bufferFloodError("foobar"); EXPECT_FALSE(status.ok()); EXPECT_EQ("foobar", status.message()); - EXPECT_EQ("BufferFloodError: foobar", ToString(status)); - EXPECT_EQ(StatusCode::BufferFloodError, GetStatusCode(status)); - EXPECT_FALSE(IsCodecProtocolError(status)); - EXPECT_TRUE(IsBufferFloodError(status)); - EXPECT_FALSE(IsPrematureResponseError(status)); - EXPECT_FALSE(IsCodecClientError(status)); + EXPECT_EQ("BufferFloodError: foobar", toString(status)); + EXPECT_EQ(StatusCode::BufferFloodError, getStatusCode(status)); + EXPECT_FALSE(isCodecProtocolError(status)); + EXPECT_TRUE(isBufferFloodError(status)); + EXPECT_FALSE(isPrematureResponseError(status)); + EXPECT_FALSE(isCodecClientError(status)); } TEST(Status, PrematureResponseError) { - auto status = PrematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired); + auto status = prematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired); EXPECT_FALSE(status.ok()); EXPECT_EQ("foobar", status.message()); - EXPECT_EQ("PrematureResponseError: HTTP code: 407: foobar", ToString(status)); - EXPECT_EQ(StatusCode::PrematureResponseError, GetStatusCode(status)); - EXPECT_FALSE(IsCodecProtocolError(status)); - EXPECT_FALSE(IsBufferFloodError(status)); - EXPECT_TRUE(IsPrematureResponseError(status)); - EXPECT_EQ(Http::Code::ProxyAuthenticationRequired, GetPrematureResponseHttpCode(status)); - EXPECT_FALSE(IsCodecClientError(status)); + EXPECT_EQ("PrematureResponseError: HTTP code: 407: foobar", toString(status)); + EXPECT_EQ(StatusCode::PrematureResponseError, getStatusCode(status)); + EXPECT_FALSE(isCodecProtocolError(status)); + EXPECT_FALSE(isBufferFloodError(status)); + EXPECT_TRUE(isPrematureResponseError(status)); + EXPECT_EQ(Http::Code::ProxyAuthenticationRequired, getPrematureResponseHttpCode(status)); + EXPECT_FALSE(isCodecClientError(status)); +} + +TEST(Status, getPrematureResponseHttpCodeWithWrongStatus) { + auto status = codecClientError("foobar"); + EXPECT_DEATH(getPrematureResponseHttpCode(status), ""); } TEST(Status, CodecClientError) { - auto status = CodecClientError("foobar"); + auto status = codecClientError("foobar"); EXPECT_FALSE(status.ok()); EXPECT_EQ("foobar", status.message()); - EXPECT_EQ("CodecClientError: foobar", ToString(status)); - EXPECT_EQ(StatusCode::CodecClientError, GetStatusCode(status)); - EXPECT_FALSE(IsCodecProtocolError(status)); - EXPECT_FALSE(IsBufferFloodError(status)); - EXPECT_FALSE(IsPrematureResponseError(status)); - EXPECT_TRUE(IsCodecClientError(status)); + EXPECT_EQ("CodecClientError: foobar", toString(status)); + EXPECT_EQ(StatusCode::CodecClientError, getStatusCode(status)); + EXPECT_FALSE(isCodecProtocolError(status)); + EXPECT_FALSE(isBufferFloodError(status)); + EXPECT_FALSE(isPrematureResponseError(status)); + EXPECT_TRUE(isCodecClientError(status)); } } // namespace Envoy diff --git a/test/common/common/statusor_test.cc b/test/common/common/statusor_test.cc index a7b4993d8f7b0..4f23242ba4ee3 100644 --- a/test/common/common/statusor_test.cc +++ b/test/common/common/statusor_test.cc @@ -7,17 +7,17 @@ namespace Envoy { TEST(StatusOr, Initialization) { - StatusOr statusor(PrematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired)); + StatusOr statusor(prematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired)); EXPECT_FALSE(statusor.ok()); - EXPECT_TRUE(IsPrematureResponseError(statusor.status())); + EXPECT_TRUE(isPrematureResponseError(statusor.status())); EXPECT_EQ("foobar", statusor.status().message()); EXPECT_EQ(Http::Code::ProxyAuthenticationRequired, - GetPrematureResponseHttpCode(statusor.status())); + getPrematureResponseHttpCode(statusor.status())); } TEST(StatusOr, DefaultInitialization) { StatusOr statusor; - EXPECT_DEATH(GetStatusCode(statusor.status()), ""); + EXPECT_DEATH(getStatusCode(statusor.status()), ""); } } // namespace Envoy From 9a81b9fb22d7ef0978dbcc11dedb09ac39fcc75e Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 10 Apr 2020 12:23:52 -0400 Subject: [PATCH 09/11] Address comments Signed-off-by: Yan Avlasov --- source/common/common/BUILD | 12 ------------ source/common/common/statusor.h | 2 +- source/common/http/BUILD | 12 ++++++++++++ source/common/{common => http}/status.cc | 10 ++++++---- source/common/{common => http}/status.h | 19 +++++++++++++++++++ test/common/common/BUILD | 10 +--------- test/common/common/statusor_test.cc | 11 ++++++----- test/common/http/BUILD | 8 ++++++++ test/common/{common => http}/status_test.cc | 4 +++- 9 files changed, 56 insertions(+), 32 deletions(-) rename source/common/{common => http}/status.cc (93%) rename source/common/{common => http}/status.h (86%) rename test/common/{common => http}/status_test.cc (97%) diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 6952c325308b5..af25042d18c50 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -430,18 +430,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "status_lib", - srcs = ["status.cc"], - hdrs = ["status.h"], - external_deps = [ - "abseil_status", - ], - deps = [ - "//include/envoy/http:codes_interface", - ], -) - envoy_cc_library( name = "statusor_lib", hdrs = ["statusor.h"], diff --git a/source/common/common/statusor.h b/source/common/common/statusor.h index 3fe21ccb9a7a8..bb7201d3c4b60 100644 --- a/source/common/common/statusor.h +++ b/source/common/common/statusor.h @@ -37,6 +37,6 @@ namespace Envoy { -using absl::StatusOr; // NOLINT: disable misc-unused-using-decls clang-tidy check +using absl::StatusOr; // NOLINT(misc-unused-using-decls) } // namespace Envoy diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 3deaff82c3052..b2b394ae7d518 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -412,3 +412,15 @@ envoy_cc_library( "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", ], ) + +envoy_cc_library( + name = "status_lib", + srcs = ["status.cc"], + hdrs = ["status.h"], + external_deps = [ + "abseil_status", + ], + deps = [ + "//include/envoy/http:codes_interface", + ], +) diff --git a/source/common/common/status.cc b/source/common/http/status.cc similarity index 93% rename from source/common/common/status.cc rename to source/common/http/status.cc index 726173ebd02f4..fb506038557ac 100644 --- a/source/common/common/status.cc +++ b/source/common/http/status.cc @@ -1,12 +1,13 @@ -#include "common/common/status.h" +#include "common/http/status.h" #include "absl/strings/str_cat.h" namespace Envoy { +namespace Http { namespace { -const char EnvoyPayloadUrl[] = {"Envoy"}; +constexpr absl::string_view EnvoyPayloadUrl = "Envoy"; absl::string_view statusCodeToString(StatusCode code) { switch (code) { @@ -42,9 +43,9 @@ template void storePayload(absl::Status& status, const T& payload) template const T* getPayload(const absl::Status& status) { auto payload = status.GetPayload(EnvoyPayloadUrl); - RELEASE_ASSERT(payload.has_value(), "Must have payload"); + ASSERT(payload.has_value(), "Must have payload"); auto data = payload.value().Flatten(); - RELEASE_ASSERT(data.length() >= sizeof(T), "Invalid payload length"); + ASSERT(data.length() >= sizeof(T), "Invalid payload length"); return reinterpret_cast(data.data()); } @@ -118,4 +119,5 @@ bool isCodecClientError(const Status& status) { return getStatusCode(status) == StatusCode::CodecClientError; } +} // namespace Http } // namespace Envoy diff --git a/source/common/common/status.h b/source/common/http/status.h similarity index 86% rename from source/common/common/status.h rename to source/common/http/status.h index 2b2f89f16057c..3efc54ee1a2f0 100644 --- a/source/common/common/status.h +++ b/source/common/http/status.h @@ -41,15 +41,33 @@ */ namespace Envoy { +namespace Http { /** * Status codes for representing classes of Envoy errors. */ enum class StatusCode : int { Ok = 0, + + /** + * Indicates a non-recoverable protocol error that should result in connection termination. + */ CodecProtocolError = 1, + + /** + * Indicates detection of outbound frame queue flood. + */ BufferFloodError = 2, + + /** + * Indicates a response is received on a connection that did not send a request. In practice + * this can only happen on HTTP/1.1 connections. + */ PrematureResponseError = 3, + + /** + * Indicates a client (local) side error which should not happen. + */ CodecClientError = 4 }; @@ -91,4 +109,5 @@ ABSL_MUST_USE_RESULT bool isCodecClientError(const Status& status); */ Http::Code getPrematureResponseHttpCode(const Status& status); +} // namespace Http } // namespace Envoy diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 23a33bfaef5b8..288afab64682a 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -266,19 +266,11 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "status_test", - srcs = ["status_test.cc"], - deps = [ - "//source/common/common:status_lib", - ], -) - envoy_cc_test( name = "statusor_test", srcs = ["statusor_test.cc"], deps = [ - "//source/common/common:status_lib", "//source/common/common:statusor_lib", + "//source/common/http:status_lib", ], ) diff --git a/test/common/common/statusor_test.cc b/test/common/common/statusor_test.cc index 4f23242ba4ee3..8f9bb42e0da77 100644 --- a/test/common/common/statusor_test.cc +++ b/test/common/common/statusor_test.cc @@ -1,5 +1,5 @@ -#include "common/common/status.h" #include "common/common/statusor.h" +#include "common/http/status.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -7,17 +7,18 @@ namespace Envoy { TEST(StatusOr, Initialization) { - StatusOr statusor(prematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired)); + StatusOr statusor( + Http::prematureResponseError("foobar", Http::Code::ProxyAuthenticationRequired)); EXPECT_FALSE(statusor.ok()); - EXPECT_TRUE(isPrematureResponseError(statusor.status())); + EXPECT_TRUE(Http::isPrematureResponseError(statusor.status())); EXPECT_EQ("foobar", statusor.status().message()); EXPECT_EQ(Http::Code::ProxyAuthenticationRequired, - getPrematureResponseHttpCode(statusor.status())); + Http::getPrematureResponseHttpCode(statusor.status())); } TEST(StatusOr, DefaultInitialization) { StatusOr statusor; - EXPECT_DEATH(getStatusCode(statusor.status()), ""); + EXPECT_DEATH(Http::getStatusCode(statusor.status()), ""); } } // namespace Envoy diff --git a/test/common/http/BUILD b/test/common/http/BUILD index dce641d6475c3..5382d74c085ca 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -384,3 +384,11 @@ envoy_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_cc_test( + name = "status_test", + srcs = ["status_test.cc"], + deps = [ + "//source/common/http:status_lib", + ], +) diff --git a/test/common/common/status_test.cc b/test/common/http/status_test.cc similarity index 97% rename from test/common/common/status_test.cc rename to test/common/http/status_test.cc index c25641a0fbaec..19cf6458d1b8e 100644 --- a/test/common/common/status_test.cc +++ b/test/common/http/status_test.cc @@ -1,9 +1,10 @@ -#include "common/common/status.h" +#include "common/http/status.h" #include "gmock/gmock.h" #include "gtest/gtest.h" namespace Envoy { +namespace Http { TEST(Status, Ok) { auto status = okStatus(); @@ -71,4 +72,5 @@ TEST(Status, CodecClientError) { EXPECT_TRUE(isCodecClientError(status)); } +} // namespace Http } // namespace Envoy From 8de3e339232e58568bfd1e1483b57d60fe223abf Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 10 Apr 2020 12:31:25 -0400 Subject: [PATCH 10/11] Address comments Signed-off-by: Yan Avlasov --- source/common/http/status.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/http/status.cc b/source/common/http/status.cc index fb506038557ac..96aa910bd9344 100644 --- a/source/common/http/status.cc +++ b/source/common/http/status.cc @@ -26,13 +26,13 @@ absl::string_view statusCodeToString(StatusCode code) { struct EnvoyStatusPayload { EnvoyStatusPayload(StatusCode status_code) : status_code_(status_code) {} - StatusCode status_code_; + const StatusCode status_code_; }; struct PrematureResponsePayload : public EnvoyStatusPayload { PrematureResponsePayload(Http::Code http_code) : EnvoyStatusPayload(StatusCode::PrematureResponseError), http_code_(http_code) {} - Http::Code http_code_; + const Http::Code http_code_; }; template void storePayload(absl::Status& status, const T& payload) { @@ -110,8 +110,8 @@ bool isPrematureResponseError(const Status& status) { Http::Code getPrematureResponseHttpCode(const Status& status) { const auto* payload = getPayload(status); - RELEASE_ASSERT(payload->status_code_ == StatusCode::PrematureResponseError, - "Must be PrematureResponseError"); + ASSERT(payload->status_code_ == StatusCode::PrematureResponseError, + "Must be PrematureResponseError"); return payload->http_code_; } From cbfac5a203639884d5a5421430f841b5fcda4e8d Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Fri, 10 Apr 2020 13:40:12 -0400 Subject: [PATCH 11/11] Remove DEATH tests after removing RELEASE_ASSERTs Signed-off-by: Yan Avlasov --- test/common/common/statusor_test.cc | 5 ----- test/common/http/status_test.cc | 5 ----- 2 files changed, 10 deletions(-) diff --git a/test/common/common/statusor_test.cc b/test/common/common/statusor_test.cc index 8f9bb42e0da77..2b42e3c6bf67a 100644 --- a/test/common/common/statusor_test.cc +++ b/test/common/common/statusor_test.cc @@ -16,9 +16,4 @@ TEST(StatusOr, Initialization) { Http::getPrematureResponseHttpCode(statusor.status())); } -TEST(StatusOr, DefaultInitialization) { - StatusOr statusor; - EXPECT_DEATH(Http::getStatusCode(statusor.status()), ""); -} - } // namespace Envoy diff --git a/test/common/http/status_test.cc b/test/common/http/status_test.cc index 19cf6458d1b8e..4783b64dd0909 100644 --- a/test/common/http/status_test.cc +++ b/test/common/http/status_test.cc @@ -55,11 +55,6 @@ TEST(Status, PrematureResponseError) { EXPECT_FALSE(isCodecClientError(status)); } -TEST(Status, getPrematureResponseHttpCodeWithWrongStatus) { - auto status = codecClientError("foobar"); - EXPECT_DEATH(getPrematureResponseHttpCode(status), ""); -} - TEST(Status, CodecClientError) { auto status = codecClientError("foobar"); EXPECT_FALSE(status.ok());