Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add StatusOr for error handling in modern C++ style #768

Merged
merged 7 commits into from
Aug 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/redis_cmd.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class CommandNamespace : public Commander {
} else {
std::string token;
auto s = config->GetNamespace(args_[2], &token);
if (s.IsNotFound()) {
if (s.Is<Status::NotFound>()) {
*output = Redis::NilString();
} else {
*output = Redis::BulkString(token);
Expand Down
2 changes: 1 addition & 1 deletion src/redis_connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ void Connection::ExecuteCommands(std::deque<CommandTokens> *to_process_cmds) {

// Break the execution loop when occurring the blocking command like BLPOP or BRPOP,
// it will suspend the connection and wait for the wakeup signal.
if (s.IsBlockingCommand()) {
if (s.Is<Status::BlockingCmd>()) {
break;
}
// Reply for MULTI
Expand Down
211 changes: 200 additions & 11 deletions src/status.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,17 @@
#pragma once

#include <string>
#include <type_traits>
#include <utility>
#include <memory>
#include <algorithm>
#include <tuple>
#include <glog/logging.h>

class Status {
public:
enum Code {
cOK,
enum Code : unsigned char {
cOK = 0,
NotOK,
NotFound,

Expand Down Expand Up @@ -61,20 +66,204 @@ class Status {
};

Status() : Status(cOK) {}
explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
bool IsBlockingCommand() { return code_ == BlockingCmd; }
std::string Msg() {
if (IsOK()) {
return "ok";
}
explicit Status(Code code, std::string msg = {})
: code_(code), msg_(std::move(msg)) {}

template <Code code>
bool Is() const { return code_ == code; }

bool IsOK() const { return Is<cOK>(); }
operator bool() const { return IsOK(); }

Code GetCode() const {
return code_;
}

std::string Msg() const& {
if (*this) return ok_msg();
return msg_;
}

std::string Msg() && {
if (*this) return ok_msg();
return std::move(msg_);
}

static Status OK() { return {}; }

private:
Code code_;
std::string msg_;

static constexpr const char* ok_msg() {
return "ok";
}

template <typename T>
friend struct StatusOr;
};

template <typename ...Ts>
using first_element = typename std::tuple_element<0, std::tuple<Ts...>>::type;

template <typename T>
using remove_cvref_t = typename std::remove_cv<typename std::remove_reference<T>::type>::type;

template <typename T>
struct StatusOr;

template <typename T>
struct IsStatusOr : std::integral_constant<bool, false> {};

template <typename T>
struct IsStatusOr<StatusOr<T>> : std::integral_constant<bool, true> {};

template <typename T>
struct StatusOr {
static_assert(!std::is_same<T, Status>::value, "value_type cannot be Status");
static_assert(!std::is_same<T, Status::Code>::value, "value_type cannot be Status::Code");
static_assert(!IsStatusOr<T>::value, "value_type cannot be StatusOr");
static_assert(!std::is_reference<T>::value, "value_type cannot be reference");

using value_type = T;

// we use std::unique_ptr to make the error part as small as enough
using error_type = std::unique_ptr<std::string>;

using Code = Status::Code;

explicit StatusOr(Status s) : code_(s.code_) {
CHECK(!s);
new(value_or_error_) error_type(new std::string(std::move(s.msg_)));
}

StatusOr(Code code, std::string msg = {}) : code_(code) { // NOLINT
CHECK(code != Code::cOK);
new(value_or_error_) error_type(new std::string(std::move(msg)));
}

template <typename ...Ts,
typename std::enable_if<
(sizeof...(Ts) > 0 &&
!std::is_same<Status, remove_cvref_t<first_element<Ts...>>>::value &&
!std::is_same<Code, remove_cvref_t<first_element<Ts...>>>::value &&
!std::is_same<value_type, remove_cvref_t<first_element<Ts...>>>::value &&
!std::is_same<StatusOr, remove_cvref_t<first_element<Ts...>>>::value
), int>::type = 0> // NOLINT
explicit StatusOr(Ts && ... args) : code_(Code::cOK) {
new(value_or_error_) value_type(std::forward<Ts>(args)...);
}

StatusOr(T&& value) : code_(Code::cOK) { // NOLINT
new(value_or_error_) value_type(std::move(value));
}

StatusOr(const T& value) : code_(Code::cOK) { // NOLINT
new(value_or_error_) value_type(value);
}

StatusOr(const StatusOr&) = delete;
StatusOr(StatusOr&& other) : code_(other.code_) {
if (code_ == Code::cOK) {
new(value_or_error_) value_type(std::move(other.getValue()));
} else {
new(value_or_error_) error_type(std::move(other.getError()));
}
}

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

template <Code code>
bool Is() const { return code_ == code; }

bool IsOK() const { return Is<Code::cOK>(); }
operator bool() const { return IsOK(); }

Status ToStatus() const& {
if (*this) return Status::OK();
return Status(code_, *getError());
}

Status ToStatus() && {
if (*this) return Status::OK();
return Status(code_, std::move(*getError()));
}

Code GetCode() const {
return code_;
}

value_type& GetValue() & {
CHECK(*this);
return getValue();
}

value_type&& GetValue() && {
CHECK(*this);
return std::move(getValue());
}

const value_type& GetValue() const& {
CHECK(*this);
return getValue();
}

value_type& operator*() & {
return GetValue();
}

value_type&& operator*() && {
return std::move(GetValue());
}

const value_type& operator*() const& {
return GetValue();
}

value_type* operator->() {
return &GetValue();
}

const value_type* operator->() const {
return &GetValue();
}

std::string Msg() const& {
if (*this) return Status::ok_msg();
return *getError();
}

std::string Msg() && {
if (*this) return Status::ok_msg();
return std::move(*getError());
}

~StatusOr() {
if (*this) {
getValue().~value_type();
} else {
getError().~error_type();
}
}

private:
Status::Code code_;
alignas(value_type) alignas(error_type) unsigned char value_or_error_
[sizeof(value_type) < sizeof(error_type) ? sizeof(error_type) : sizeof(value_type)];

value_type& getValue() {
return *reinterpret_cast<value_type*>(value_or_error_);
}

const value_type& getValue() const {
return *reinterpret_cast<const value_type*>(value_or_error_);
}

error_type& getError() {
return *reinterpret_cast<error_type*>(value_or_error_);
}

const error_type& getError() const {
return *reinterpret_cast<const error_type*>(value_or_error_);
}
};
146 changes: 146 additions & 0 deletions tests/cppunit/status_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you 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 <gtest/gtest.h>

#include <memory>
#include <status.h>

TEST(StatusOr, Scalar) {
auto f = [](int x) -> StatusOr<int> {
if (x > 10) {
return {Status::NotOK, "x large than 10"};
}

return 2 * x + 5;
};

ASSERT_EQ(*f(1), 7);
ASSERT_EQ(*f(5), 15);
ASSERT_EQ(f(7).GetValue(), 19);
ASSERT_EQ(f(7).GetCode(), Status::cOK);
ASSERT_EQ(f(7).Msg(), "ok");
ASSERT_TRUE(f(6));
ASSERT_EQ(f(11).GetCode(), Status::NotOK);
ASSERT_EQ(f(11).Msg(), "x large than 10");
Copy link
Member

Choose a reason for hiding this comment

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

what would happen if calling f(11).GetValue()?

Copy link
Member Author

@PragmaTwice PragmaTwice Aug 5, 2022

Choose a reason for hiding this comment

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

Hi @ShooterIT, if you look at the definition of StatusOr<T>::GetValue:

  value_type& GetValue() & {
    CHECK(*this);
    return getValue();
  }

  value_type&& GetValue() && {
    CHECK(*this);
    return std::move(getValue());
  }

  const value_type& GetValue() const& {
    CHECK(*this);
    return getValue();
  }

You will find there is three different overload of this method via changing the qualifier of *this.

Hence, in f(x).GetValue(), obviously f(x) is a prvalue since a temporary object is returned, and then the signature value_type&& GetValue() && is decided to be called after the overload resolution, and thus you get an xvalue (which is a special kind of rvalue) referenced to the underlying value of the returned StatusOr.

So we should take care of the lifetime in this case, the rvalue will become invalid after the computation of the current statement. We can extend it via a call to a move ctor (T(T&&)). This is not a defect of this type, but it is an idiom of C++, we should always care about lifetime of references to avoid dangling references in all standards of C++ (98/03/11/14/17/20 ...).

Copy link
Member Author

@PragmaTwice PragmaTwice Aug 5, 2022

Choose a reason for hiding this comment

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

BTW, the classic usage of StatusOr<T> we expected is:

// 1
StatusOr<T> someProcess(...); // as a return type

// 2
T val;
...
return val; // return val typed T to StatusOr<T>

// 3
return {Status::someErrCode, "msg..."}; // return an error status

// 4
auto res = someProcess(...); // get result typed StatusOr<T>
if(!res) return res; // forward it if it is an error status

doSomething(*res); // process the value inside the StatusOr

Use StatusOr as an rvalue is totally (well) supported, but everyone should be aware of what they are doing, otherwise just follow the classic usage is fine.

Copy link
Member

Choose a reason for hiding this comment

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

i want to describe 11 is greater than 10, GetValue() will return error?

Copy link
Member Author

@PragmaTwice PragmaTwice Aug 12, 2022

Choose a reason for hiding this comment

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

GetValue() will trigger a fatal error.
It is just like you call f as Status f(int *output), but do not check the returned Status and directly dereference the output. It is not allowed, you must check status before dereference.

Copy link
Member

Choose a reason for hiding this comment

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

I think that guarded by CHECK(*this), it can cause a fatal error.

I wonder if we can generate a compile time error instead of runtime fatal in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Confirm its failure as:

WARNING: Logging before InitGoogleLogging() is written to STDERR
F20220812 15:30:19.518088 1124890 status.h:202] Check failed: *this 
*** Check failure stack trace: ***

Copy link
Member Author

@PragmaTwice PragmaTwice Aug 16, 2022

Choose a reason for hiding this comment

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

Hi @tisonkun, thanks for your review.

I wonder if we can generate a compile time error instead of runtime fatal in this case.

Good idea. We consider an example for this:

auto res = someProcess(...); // get result typed StatusOr<T>
// we cannot deref `res` here

if(!res) { 
  processError(res.GetCode(), res.Msg()); /* or */ return res; // we cannot deref `res` here
}

doSomething(*res); // we can deref it

In this example, only the doSomething part can actually dereference the res.
We cannot dereference it both before if(!res) and inside if(!res).
So only in the execution path with the condition res.IsOk() == true being satisfied, we can dereference res,

It is hardly to express in compile time, since it is related to the control flow.
Like a raw/smart pointer, we should check it does point to a value (not null) before dereference it.

I have implemented a checker in Clang Static Analyzer (included in clang-tidy) for OneFlow that checks whether a type like StatusOr is dereferenced after checking IsOk() using symbolic execution, and I think it can be done in this way in the future.

Copy link
Member

@tisonkun tisonkun Aug 16, 2022

Choose a reason for hiding this comment

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

checks whether a type like StatusOr is dereferenced after checking IsOk() using symbolic execution

Sounds good to me.

ASSERT_FALSE(f(12));

auto x = f(5);
ASSERT_EQ(*x, 15);
ASSERT_EQ(x.Msg(), "ok");
ASSERT_EQ(x.GetValue(), 15);
ASSERT_EQ(x.GetCode(), Status::cOK);

auto y = f(11);
ASSERT_EQ(y.Msg(), "x large than 10");
ASSERT_EQ(y.GetCode(), Status::NotOK);

auto g = [f](int y) -> StatusOr<int> {
if (y > 5 && y < 15) {
return {Status::NotOK, "y large than 5"};
}

auto res = f(y);
if (!res) return res;

return *res * 10;
};

ASSERT_EQ(*g(1), 70);
ASSERT_EQ(*g(5), 150);
ASSERT_EQ(g(1).GetValue(), 70);
ASSERT_EQ(g(1).GetCode(), Status::cOK);
ASSERT_EQ(g(1).Msg(), "ok");
ASSERT_EQ(g(6).GetCode(), Status::NotOK);
ASSERT_EQ(g(6).Msg(), "y large than 5");
ASSERT_EQ(g(20).GetCode(), Status::NotOK);
ASSERT_EQ(g(20).Msg(), "x large than 10");
ASSERT_EQ(g(11).GetCode(), Status::NotOK);
ASSERT_EQ(g(11).Msg(), "y large than 5");
}

TEST(StatusOr, String) {
auto f = [](std::string x) -> StatusOr<std::string> {
if (x.size() > 10) {
return {Status::NotOK, "string too long"};
}

return x + " hello";
};

auto g = [f](std::string x) -> StatusOr<std::string> {
if (x.size() < 5) {
return {Status::NotOK, "string too short"};
}

auto res = f(x);
if (!res) return res;

return "hi " + *res;
};

ASSERT_TRUE(f("1"));
ASSERT_FALSE(f("12345678901"));
ASSERT_FALSE(g("1"));

ASSERT_EQ(*f("twice"), "twice hello");
ASSERT_EQ(*g("twice"), "hi twice hello");
ASSERT_EQ(g("shrt").GetCode(), Status::NotOK);
ASSERT_EQ(g("shrt").Msg(), "string too short");
ASSERT_EQ(g("loooooooooooog").GetCode(), Status::NotOK);
ASSERT_EQ(g("loooooooooooog").Msg(), "string too long");

ASSERT_EQ(g("twice").ToStatus().GetCode(), Status::cOK);
ASSERT_EQ(g("").ToStatus().GetCode(), Status::NotOK);

auto x = g("twice");
ASSERT_EQ(x.ToStatus().GetCode(), Status::cOK);
auto y = g("");
ASSERT_EQ(y.ToStatus().GetCode(), Status::NotOK);
}

TEST(StatusOr, SharedPtr) {
struct A {
A(int *x) : x(x) { *x = 233; }
~A() { *x = 0; }

int *x;
};

int val = 0;

{
StatusOr<std::shared_ptr<A>> x(new A(&val));

ASSERT_EQ(val, 233);
ASSERT_EQ(x->use_count(), 1);

{
StatusOr<std::shared_ptr<A>> y(*x);
ASSERT_EQ(val, 233);
ASSERT_EQ(x->use_count(), 2);
}

ASSERT_EQ(x->use_count(), 1);
}

ASSERT_EQ(val, 0);

}