Add Status and StatusOr classes #10550

yanavlasov · 2020-03-26T23:38:24Z

Add Status and StatusOr classes.
These classes are to be used for removing exceptions in HTTP codecs.
The Status class is for returning reach error information, similar to exception types. It is an alias of absl::Status.
The StatusOr class is a fork of not yet open source absl::StatusOr class. Once absl releases StatusOr the fork will be removed.

Risk Level: None
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Yan Avlasov yavlasov@google.com

These classes are to be used for removing exceptions in HTTP codecs. Signed-off-by: Yan Avlasov <yavlasov@google.com>

Signed-off-by: Yan Avlasov <yavlasov@google.com>

asraa

Thank you!! Some initial comments...

source/common/http/status.h

mattklein123

Thanks for working on this. I have a high level question before digging into the details.

/wait-any

source/common/http/status.h

kyessenov · 2020-03-27T21:16:41Z

source/common/http/status.h

+ * Constructing an error status is likely as heavy as throwing an exception due to a heap
+ * allocation.
+ */
+class ABSL_MUST_USE_RESULT Status final {


Should we at least attempt to re-use abseil status and use the status payload for application-specific errors?
Having multiple statuses is a real pain.

+1, I'd really like to understand what absl::Status is lacking and see if we can resolve that.

Hm I see. We could potentially map our five (OK, CodecProtocol, BufferFlood, etc) to five of the absl canonical errors, and set up a kHttpCode payload to hold the HttpCode in the case of a PrematureResponseException.

Other things we want are:

a StatusOr -- maybe we need in-house absl::variant holding absl::Status that can be a drop in when absl::StatusOr is released?

the potential to prevent overwriting a bad status (we catch and set a codec error, ASSERT we don't try to set it again).

See my comment above also? Would all of this be solved via a template parameter/struct (which could be empty) added to the underlying status class?

re: StatusOr -- it is about to be open sourced (probably weeks, not months). For now, the recommendation is to fork it (e.g. https://github.com/google/cel-cpp/tree/master/base), and let the abseil team know. Using abseil standard status should provide better experience with testing macros, proto/grpc status alignment, etc down the line I'd think.

I've spent some time talking to the absl engineer about how to make absl::Status work with custom error spaces and here are the takeaways.

@ahedberg The only question is how to reconsile absl::Status error space with the error space of Envoy dataplane.

@mattklein123 It is highly unlikely that there will be ability to change error codes of absl::Status by any means. It is tied to the "canonical Google error space" and flexibility of changing it is outside of the scope of this class. This was actually considered before and discarded as it was adding unneeded (internally to Google) complexity.

Recommended and the only way to modify error codes is to use absl::Status payloads which is a set of key/value pairs. Value is an opaque byte buffer. As suggested by @kyessenov

So the API to the error facility might look something like this if we go with the absl::Status

namespace Envoy { using Status = absl::Status; enum class StatusCode : int { CodecProtocolError = 0, BufferFloodError = 1, PrematureResponseError = 2 }; // Methods for creating error codes Status MakeCodecProtocolError(absl::string_view message); Status MakeBufferFloodError(absl::string_view message); Status MakePrematureResponseError(absl::string_view message, Http::Code http_code); // Methods for checking and extracting error information StatusCode GetStatusCode(Status); bool IsCodecProtocolError(Status); // ... } // namespace Envoy

Internally these methods will create absl::Status with e.g. "Internal" error code and a payload with Envoy's error code and optionally the http_code. The extractors will get the error details from the payload. We could also do error code mapping as @asraa suggested. This would make allocation cost go up for one class of error only.

Downsides are:

Creating a payload needs two allocations

Extracting may need one too, unless we want to deal with the absl::Cord iterators and reconstruct payload byte by byte.

absl::Status can not be constructed directly and its code() method always return the same error code. It may be a gotcha for someone who does not know that there is additional API to be used for constructing and checking error codes.

The upside is that:

We do not need to maintain absl::Status. Although we will need to maintain the payload serializer/deserializer code.

absl::StatusOr comes for free as well as it will use absl::Status

Let me know your thoughts. It seems like people want to avoid forking these two classes and I agree this is not ideal.

IMO the upsides of reusing the existing classes outweigh the negatives, especially because in the happy path there would be no extra allocations, so it seems fine to me.

Conceptually though, I still don't understand why the payload mechanism couldn't be templated in such a way where the default implementation is the key/value pair map, and then we could have our own implementation with our own well specified struct. This would force StatusOr to also be templated on our templated Status type. This seems like the optimal solution, albeit a bunch more work, so I suppose it probably would not happen.

Signed-off-by: Yan Avlasov <yavlasov@google.com>

mattklein123

Thanks this looks like it's on the right track. Thanks for iterating on not forking more code than we need to. A few high level comments/questions.

/wait

source/common/common/status.cc

mattklein123 · 2020-04-06T22:56:13Z

source/common/common/status.cc

+  }
+}
+
+StatusCode ToEnvoyStatusCode(absl::StatusCode code) {


The below mappings seem kind of strange. Do we need these mappings?

Edit: I see that this is a mapping to attempt not using internal error in all cases, is that right? If so maybe add a bunch more comments? This seems fragile and I wonder if we should just always take the perf hit to use the embeded payload to describe the status? wdyt?

I personally think this is OK, can we add a table at the top of the header file, and comments in the enum definition for StatusCode? I think people do generally map their codes on to the canonical codes, eg (https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto).

I've changed it to use payloads in all cases, just for comparison. Check if it is better.

source/common/common/status.h

source/common/common/statusor.cc

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Exclude third_party from check_format Signed-off-by: Yan Avlasov <yavlasov@google.com>

Signed-off-by: Yan Avlasov <yavlasov@google.com>

source/common/common/status.cc

lizan · 2020-04-09T20:23:14Z

source/common/common/statusor.h

+
+namespace Envoy {
+
+using absl::StatusOr; // NOLINT: disable misc-unused-using-decls clang-tidy check


use NOLINT(misc-unused-using-decls)

Do we need this file at all? Can't we temporarily just include the third party file directly until we get it from absl?

I was thinking that it would make transition from fork to released version easier. Otherwise a bunch files would need to have include paths and BUILD dependencies changed. absl does not have a timeline for releasing StatusOr it can be awhile.

mattklein123

Thanks generally looks good to me. A few small comments and will defer to @asraa and others for further review.

/wait

source/common/common/status.cc

source/common/common/status.h

mattklein123 · 2020-04-09T23:40:20Z

source/common/common/statusor.h

+
+namespace Envoy {
+
+using absl::StatusOr; // NOLINT: disable misc-unused-using-decls clang-tidy check


Do we need this file at all? Can't we temporarily just include the third party file directly until we get it from absl?

mattklein123 · 2020-04-09T23:40:52Z

third_party/statusor/statusor_test.cc

@@ -0,0 +1,441 @@
+/**


I doubt this is going to get run so is it worth copying?

I've renamed some methods of the forked StatusOr public API after talking to absl engineer to match what is planned to be open sourced. The file is forked from tensorflow and API names were outdated. So I needed the test to make sure I did not screw something up. I mean it does not hurt to keep it for now.

Sure sounds good.

asraa

This is awesome, thank you for all your work! LGTM, non-blocking comment addition

source/common/common/status.h

Signed-off-by: Yan Avlasov <yavlasov@google.com>

mattklein123

Nice!

sunjayBhatia · 2020-04-14T19:38:31Z

source/common/http/status.cc

+    return "PrematureResponseError";
+  case StatusCode::CodecClientError:
+    return "CodecClientError";
+  }


we're getting compilation issues on Windows as this helper function does not return a value in all cases, we're going to make a PR to fix this and add a RELEASE_ASSERT in the default case (and change all all the below ASSERTs to RELEASE_ASSERTs), if an exception is more appropriate, please let us know

The documentation of getStatusCode mentions a RELEASE_ASSERT but is using ASSERT

Disregard the part about ASSERT -> RELEASE_ASSERT as I see the comments now asking to do the opposite

Add Status and StatusOr classes.

5efd11a

These classes are to be used for removing exceptions in HTTP codecs. Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov assigned asraa and mattklein123 Mar 27, 2020

Mollify clang-tidy

9bd9d7f

Signed-off-by: Yan Avlasov <yavlasov@google.com>

asraa reviewed Mar 27, 2020

View reviewed changes

mattklein123 reviewed Mar 27, 2020

View reviewed changes

source/common/http/status.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting:any label Mar 27, 2020

kyessenov reviewed Mar 27, 2020

View reviewed changes

repokitteh-read-only bot removed waiting:any labels Mar 27, 2020

mattklein123 added the waiting label Mar 27, 2020

Address comments

ed3cea4

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot removed the waiting label Apr 6, 2020

yanavlasov changed the title ~~Add Status and StatusOr classes.~~ Add Status class Apr 6, 2020

yanavlasov added 2 commits April 6, 2020 11:39

Merge branch 'master' into statusor

cdc5b3c

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Fork absl::StatusOr

01d1739

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov changed the title ~~Add Status class~~ Add Status and StatusOr classes Apr 6, 2020

mattklein123 requested changes Apr 6, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 6, 2020

yanavlasov added 4 commits April 8, 2020 16:13

Merge branch 'master' into statusor

479c026

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Create StatusOr fork in the absl namespace

552f882

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Add Envoy::StatusOr alias and its unit tests

d857eef

Exclude third_party from check_format Signed-off-by: Yan Avlasov <yavlasov@google.com>

Exclude third_party from clang-tidy

185b002

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot removed the waiting label Apr 9, 2020

yanavlasov assigned lizan Apr 9, 2020

yanavlasov added 2 commits April 9, 2020 09:43

Merge branch 'master' into statusor

f376c73

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Address comments

25329e5

Signed-off-by: Yan Avlasov <yavlasov@google.com>

lizan reviewed Apr 9, 2020

View reviewed changes

mattklein123 requested changes Apr 9, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 9, 2020

asraa reviewed Apr 10, 2020

View reviewed changes

source/common/common/status.h Outdated Show resolved Hide resolved

Address comments

9a81b9f

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot removed the waiting label Apr 10, 2020

yanavlasov added 2 commits April 10, 2020 12:31

Address comments

8de3e33

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Remove DEATH tests after removing RELEASE_ASSERTs

cbfac5a

Signed-off-by: Yan Avlasov <yavlasov@google.com>

mattklein123 approved these changes Apr 10, 2020

View reviewed changes

lizan approved these changes Apr 10, 2020

View reviewed changes

asraa merged commit 44eedc7 into envoyproxy:master Apr 13, 2020

sunjayBhatia reviewed Apr 14, 2020

View reviewed changes

sunjayBhatia mentioned this pull request Apr 14, 2020

Windows compilation: ensure statusCodeToString has no control paths that do not return #10777

Merged

asraa mentioned this pull request Apr 21, 2020

[HTTP] High level design: Remove exceptions in codecs #10878

Closed

9 tasks

yanavlasov deleted the statusor branch July 30, 2020 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Status and StatusOr classes #10550

Add Status and StatusOr classes #10550

yanavlasov commented Mar 26, 2020 •

edited

Loading

asraa left a comment

mattklein123 left a comment

kyessenov Mar 27, 2020

ahedberg Mar 30, 2020

asraa Mar 30, 2020 •

edited

Loading

mattklein123 Mar 30, 2020

kyessenov Mar 30, 2020

yanavlasov Apr 3, 2020

mattklein123 Apr 3, 2020

mattklein123 left a comment

mattklein123 Apr 6, 2020

asraa Apr 7, 2020

yanavlasov Apr 9, 2020

lizan Apr 9, 2020

mattklein123 Apr 9, 2020

yanavlasov Apr 10, 2020 •

edited

Loading

mattklein123 left a comment

mattklein123 Apr 9, 2020

mattklein123 Apr 9, 2020

yanavlasov Apr 10, 2020 •

edited

Loading

mattklein123 Apr 10, 2020

asraa left a comment

mattklein123 left a comment

sunjayBhatia Apr 14, 2020

sunjayBhatia Apr 14, 2020

sunjayBhatia Apr 15, 2020


		namespace Envoy {

		using absl::StatusOr; // NOLINT: disable misc-unused-using-decls clang-tidy check

Add Status and StatusOr classes #10550

Add Status and StatusOr classes #10550

Conversation

yanavlasov commented Mar 26, 2020 • edited Loading

asraa left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Mar 26, 2020 •

edited

Loading

asraa Mar 30, 2020 •

edited

Loading

yanavlasov Apr 10, 2020 •

edited

Loading

yanavlasov Apr 10, 2020 •

edited

Loading