Thoughts on absl::StatusOr?

[Lei Zhang]

https://crrev.com/804225 adds an entry mentioning absl::StatusOr, but
I haven't seen any discussion on it. Any thoughts on allowing it, or
introducing a version in base/? Looking around, I already see several
versions outside of third_party/:
- TrustTokenStatusOrRequestHelper in services/network/trust_tokens/
- reporting::StatusOr in components/reporting/
- media::StatusOr in net/base/

[Leonid Baraz]

Generally speaking, I'd vote for adding all of Abseil to become available in both Chrome and ChromeOS.

Introducing it in Chrome alone seems to me counterproductive - it will drive the two source bases further away from each other.

--

Leonid Baraz

 SW Engineer

lba...@chromium.org

[Peter Kasting]

absl::Status' API loops in Cord, string_view, and optional.  At least the latter two Jan and I have ideas on how to allow eventually, but no one has proposed Cord yet.  (It, too, depends somewhat on string_view and optional.)

My proposal is, in this order:

* Migrate base::Optional to absl::optional

* Migrate base::StringPiece to be a type alias to absl::string_view, leaving StringPiece16 alone; then allow absl::string_view additionally

* Allow absl::Cord

* Allow absl::Status

* Allow absl::StatusOr

Would anyone like to help do the work on the first two bullets here, which are (hopefully) the only ones that are actually nontrivial?  We will consult with you on implementation :)

PK

[dan...@chromium.org]

On Wed, Apr 14, 2021 at 1:48 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

absl::Status' API loops in Cord, string_view, and optional.  At least the latter two Jan and I have ideas on how to allow eventually, but no one has proposed Cord yet.  (It, too, depends somewhat on string_view and optional.)

My proposal is, in this order:

* Migrate base::Optional to absl::optional

I double checked that our base::Optional CHECKs would be preserved, and it appears they all have equivalent checks in absl. Some of them are always on, and some are enabled if ABSL_OPTION_HARDENED is enabled. We appear to enable that globally with a patch against abseil: https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/patches/0001-Turn-on-hardened-mode.patch?ss=chromium%2Fchromium%2Fsrc&q=ABSL_OPTION_HARDENED

So hopefully this is just making base::Optional and base::nullopt_t into aliases to absl, then rewrite all uses.

* Migrate base::StringPiece to be a type alias to absl::string_view, leaving StringPiece16 alone; then allow absl::string_view additionally

* Allow absl::Cord

* Allow absl::Status

* Allow absl::StatusOr

Would anyone like to help do the work on the first two bullets here, which are (hopefully) the only ones that are actually nontrivial?  We will consult with you on implementation :)

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDRfYtpwbAZqr-NL8QTLGfQ01ph5paQUYxNtNucHdyv3g%40mail.gmail.com.

[Jan Wilken Dörrie]

On Wed, Apr 14, 2021 at 3:54 PM <dan...@chromium.org> wrote:

On Wed, Apr 14, 2021 at 1:48 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

absl::Status' API loops in Cord, string_view, and optional.  At least the latter two Jan and I have ideas on how to allow eventually, but no one has proposed Cord yet.  (It, too, depends somewhat on string_view and optional.)

My proposal is, in this order:

* Migrate base::Optional to absl::optional

I double checked that our base::Optional CHECKs would be preserved, and it appears they all have equivalent checks in absl. Some of them are always on, and some are enabled if ABSL_OPTION_HARDENED is enabled. We appear to enable that globally with a patch against abseil: https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/patches/0001-Turn-on-hardened-mode.patch?ss=chromium%2Fchromium%2Fsrc&q=ABSL_OPTION_HARDENED

So hopefully this is just making base::Optional and base::nullopt_t into aliases to absl, then rewrite all uses.

Yup, that matches my understanding as well (% adding some build deps on third_party/abseil-cpp:absl). Seems to be a good candidate for the next LSC :) We just need to be careful once we adopt C++17. By default absl::optional will then simply become an alias for std::optional, which as of right now does not have an option to enable extra hardening. We might want to #define ABSL_OPTION_USE_STD_OPTIONAL 0 for the time being.

 

* Migrate base::StringPiece to be a type alias to absl::string_view, leaving StringPiece16 alone; then allow absl::string_view additionally

Making base::StringPiece API compatible with {absl,std}::string_view is https://crbug.com/1049498 in case someone is interested in helping out. This is a necessary first step before we can consider making StringPiece an alias for string_view.

 

* Allow absl::Cord

* Allow absl::Status

* Allow absl::StatusOr

Would anyone like to help do the work on the first two bullets here, which are (hopefully) the only ones that are actually nontrivial?  We will consult with you on implementation :)

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDRfYtpwbAZqr-NL8QTLGfQ01ph5paQUYxNtNucHdyv3g%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQ4SRogr3vDyxHV%2B4cSHaF1HjP%2BtXsqgBjBBJAWNboEqA%40mail.gmail.com.

[Daniel Murphy]

Looks like https://crbug.com/1049498 is fixed, so is the next step is to use absl::string_view?

[Daniel Cheng]

absl::string_view isn't templated on the character type yet, which would be another necessary prerequisite. I think absl was previously opposed to this but now is open to it. I'm not sure if anyone is actively making this change already though.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/6e679470-7685-4005-9a67-e0140d76e1edn%40chromium.org.

[Giovanni Ortuño]

I looked into StatusOr for a project I'm working on. It didn't end up being a good fit and I'm not sure how well StatusOr fits within Chromium. It uses proto errors for StatusCode which might not be applicable to things in Chromium. Does anyone have thoughts about this? Are we ok with using StatusCode as is?

For the project I'm working on we decided to implement our own StatusOr which is templated on both Status and Value.

Gio

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqaZrxJr74LpMDKuRh_xvYFye6AiAbaQVU5Gm3O3drgzg%40mail.gmail.com.

[Frank Liberato]

+Ted Meyer +Xiaohan Wang 

We in media-land also looked at replacing many of our return values with absl::Status / StatusOr, and it wasn't a great fit for us, either.  Ted recently landed TypedStatus<T>, which is similar to absl::Status.  Some differences:

• T is the enum class that contains your own status codes, rather than a big global list of them.
• e.g., we have PipelineStatus and DecodeStatus, etc., that used to be raw enum values.
• result is strongly-typed, so you compile-time know what enum you're expecting
• lets you build a well-defined API -- not "can return any status code ever imagined"
• supports caused-by relationship with other TypedStatus to track where an error came from (including ones with different enum types)
• supports arbitrary (key,base::Value) pairs attached to the status, for things like windows HRESULT
• mojo-friendly
• allows an optional super cheap (sizeof(void*), no allocations, etc.) default status code, usually for "success".
• has an ::Or class, that looks similar in spirit to the StatusOr linked above.

thanks

-fl

--

I sometimes work nonstandard hours.  Please don't feel any urgency to respond outside of your working hours.

[dan...@chromium.org]

base::FileErrorOr was also recently introduced: https://source.chromium.org/chromium/chromium/src/+/main:base/files/file_error_or.h

[K. Moon]

Quite a bit of thought went into absl::StatusOr before it was open-sourced, so I'd be pretty careful before deciding that the design decisions were the wrong ones. That said, I don't have any practical experience with it to comment on the merits.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQawrf3Cg3Q5os38sY3j1EfKpTQ8kgty2dMx4Xpxcn8Eg%40mail.gmail.com.

[Peter Boström]

I'm worried about all of these variants of StatusOr popping up. Do we really have strong enough reasons to roll our own?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-5466fc-Kkn2RRa%3DnDyVqcx%2BNEyjO7TfLnm%3D-Ngk1m1cA%40mail.gmail.com.

[K. Moon]

Or at least can we only roll one of our own? :-)

[Frank Liberato]

strong enough reasons to roll our own?

This is a question that came up early in the media::TypedStatus discussion -- we very much tried to use the open-source absl::Status, but it was pretty restrictive with respect to what we wanted it for.  the "one big list of canonical statuses", in particular, was one of the big blockers for us.

was very long discussion, but TL;DR: we have many places where we return a small, specific enum of statuses (e.g., DECODE_OK, DECODE_ABORTED, DECODE_ERROR), which are actionable in the code that receives them.  we want to switch() on it, no default cases so we get compiler errors if we miss any.  often, we have a StatusOr<> for the real payload.  on error => attach a caused-by chain of what actually led up to it for logging / metrics / devtools.

this is a pattern that occurs a lot in chrome, from what we've observed.

only roll one of our own?

+1 .  we've tried very hard to make TypedStatus a starting point for something that's potentially useful beyond media.  feedback and discussion is always welcome, since we don't really know what other folks want.  :)

thanks

-fl

[Joe Mason]

Seems to me abls::Status and media::TypedStatus (and the associated StatusOr classes) both address two somewhat orthogonal concerns:

1. Providing a convenient wrapper for a result type OR error, with programming patterns to propagate the error if necessary.

2. Providing a common set of errors and an extension mechanism to attach context to them.

These aren't completely orthogonal, since once you propagate the error you must think about how other layers handle it, which is where the common error set comes in.

To address concern (2), absl::Status chose to bake a quite prescriptive error set and extension mechanism into the same class that provides (1), presumably because for the original usage in Google it was very desirable to enforce consistency in error handling across a wide codebase.(That's just my guess, I haven't spelunked through docs for the history of this decision or anything.) It uses the philosophy that higher layers should recognize the "canonical errors" and know how to handle them, so that's the only error codes that should propagate, and extension data is used both to add generic context information (eg. for logging) and to backdoor in error-specific information that cooperating higher layers might recognize and act upon.

media::TypedStatus instead makes the error set more flexible to enable the local error handling pattern Frank described, but adds a more detailed list of extension data to propagate to other layers including the ability to wrap a TypedStatus in another one (presumably with a more generic error code useful to the next layer up) and attach the original status in the extension data.

(1) is widely useful throughout Chrome, and it would be good to have a standardized class to do this. The media::TypedStatus approach of allowing different error sets in local code seems more useful for the various users in Chrome.

I think the requirements of (2) are more contentious because it depends a lot on how the extension data will be used. For instance media::TypedStatus includes a rich set of context information (code location, stack frames) so I assume there's a consumer that hooks all this up to error reporting. Is that precise method of error reporting useful outside media?

I wonder if we should split these concerns and use an evolution of media::TypedStatus as the base status class (including only status code, message, and list of wrapped "causes") and allow different users (such as media) to extend it with specific extension data. An absl::Status interop class for the third-party boundary could be one of those extensions, if needed.

That would let us immediately reign in the number of Status re-implementations without blocking on the more contentious extension data design.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CANsaiKj73xv6HSrLc8W8gA4sCDf4j9%3DmoR%3D3YN874fBm_XuTgA%40mail.gmail.com.

[K. Moon]

There's been quite a bit of informal discussion about this on the #base Slack the last few days, and danakj@ suggested bringing some of that discussion back to the mailing list thread. I'll try to summarize those discussions for this list (which might put Joe's ideas into more context; I was typing this up at the same time).

Slack participants: Please feel free to correct anything I've misrepresented.

Why absl::StatusOr might be controversial:

1. It's not really absl::StatusOr that's controversial, it's absl::Status.
2. absl::Status only directly supports a limited number of generic "canonical codes." Joe covered some of the pros and cons already. I used the analogy of HTTP status codes to argue why limited status codes are beneficial for API clients. dcheng@ brought up how many errors in Chrome APIs would just fall into the INTERNAL bucket, but that callers probably don't care in many cases. danakj@ brought up examples of rich error codes in Chrome today, like media/base/status_codes.h and base/files/file.h. There was some discussion back and forth about how to map rich error codes to canonical codes.
3. The absl::Status mechanism for supporting arbitrary error details is the payload system, which combines a URL string and an absl::Cord. With the small string optimization (SSO), both std::string and absl::Cord can support short strings (<16 bytes) without allocation. This could be used to support serializing arbitrary enums efficiently, but at a loss of type safety. I suggested this could be mitigated with usage restrictions enforced by a compiler plugin or presubmit.

On the other hand, standardizing on absl::Status provides a couple attractive benefits:

1. One Status type, not many. This is particularly important for interfacing with third party libraries, and it's easier to teach/learn.
2. absl::Status is almost trivially serializable: It's just a status code plus a map of byte strings. This would make it easy to pass to, say, Java (important for Clank, say), or across Mojo IPC.

Where the discussion ended up (Joe's post captures a fair amount of this):

1. Making absl::Status work for Chrome may still be the best compromise in the end, but we should think about what our requirements are.
2. Nobody seemed strongly opposed to defining a Chrome-specific version of Status (as long as there's just one within Chrome).
3. I proposed it would be beneficial if such a "base::Status" interoperated easily with libraries using absl::Status.
4. absl::Status interoperability implies serializability. I suggested this could be Mojo serialization (probably using a helper library, since Mojo isn't part of base), while danakj@ proposed just supporting arbitrary enums might be sufficient.
   

[K. Moon]

Correction to a point above: While SSO allows a URL + payload to be stored without additional allocation, the actual absl::Status implementation only reserves enough space for a single pointer, and allocates additional objects if the Status includes a message or payload.

[Joe Mason]

I think you missed one important point that was brought up on slack: serializing a status across IPC requires security review each time, and security generally opposes arbitrary string data unless it's guaranteed to only be used for display/logging at the far end. (Mainly to avoid serializing structured data to a string, passing it through IPC in the unconstrained string field, and then parsing it, which could break the Rule of 2. But security also likes to review the semantics of messages passed over IPC, so generic containers like base::Value are also scrutinized more strongly.)

absl::Status's extension format is exactly the arbitrary string data security generally opposes, so we wouldn't be able to send arbitrary absl::Status over IPC without careful review. (Although we could add a layer that constrains the contents of those strings to something security-approved, to make review easier.) Using base::Value as an extension format is a bit fuzzier. Using structured extension data like Mojo structs or protobufs would let security review the format of each serialized status, at the cost of constraining the data used in Status objects that are never sent across IPC. (But I suspect statuses that aren't ever sent over IPC won't often include much extension data anyway.)

If we use absl::Status directly, or something else with a fixed list of status codes, the most common extension will be to add an enum of additional domain-specific status codes (and possibly a human-readable message), so the extension system should make that special case easy and cheap.

[K. Moon]

Good points; I wouldn't think we'd allow plain absl::Status without automatic enforcement on the payload types, at a minimum. A custom type may end up being the easier path for this reason alone. I'd like to see what that actually ends up looking like, though.

[K. Moon]

(This is also an area where we'll need to be careful about what third party code does.)

[Brian Johnson]

Any thoughts on StatusOr vs base::expected? https://source.chromium.org/chromium/chromium/src/+/main:base/types/expected.h

[Peter Kasting]

On Wed, Jun 8, 2022 at 9:11 AM Brian Johnson <bjoh...@brave.com> wrote:

Any thoughts on StatusOr vs base::expected? https://source.chromium.org/chromium/chromium/src/+/main:base/types/expected.h

In general, I think expected is a better route to go for new code.  base::FileErrorOr will probably become base::expected in the future.

PK

[Leonid Baraz]

StatusOr<T> would become base::expected<T, Status> ?

If there are no semantic differences between the two, it looks like a good idea. 

[David Benjamin]

StatusOr<T> and expected<T, Status> are slightly different in that expected<T, E> only has an E in the error state. E isn't expected to contain an OK value. Status has an OK value built into it, and StatusOr handles that specially. E.g. constructing a StatusOr from a Status has this runtime check, since the type system doesn't capture the error state:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/status/internal/statusor_internal.h;drc=71ff7fa8c189cc0991f6a473e8b496843b27c116;l=216

Whereas expected<T, Status> would assume OkStatus() is a perfectly acceptable error value and happily let you return unexpected(OkStatus()).

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKPsC09XCosAMnwD3emPmO-faKX_6c8-w1rTf9D9SLrVeWcV7w%40mail.gmail.com.

[dan...@chromium.org]

On Tue, Jun 14, 2022 at 11:06 AM David Benjamin <davi...@chromium.org> wrote:

StatusOr<T> and expected<T, Status> are slightly different in that expected<T, E> only has an E in the error state. E isn't expected to contain an OK value. Status has an OK value built into it, and StatusOr handles that specially. E.g. constructing a StatusOr from a Status has this runtime check, since the type system doesn't capture the error state:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/status/internal/statusor_internal.h;drc=71ff7fa8c189cc0991f6a473e8b496843b27c116;l=216

Whereas expected<T, Status> would assume OkStatus() is a perfectly acceptable error value and happily let you return unexpected(OkStatus()).

It sounds like StatusOr should check that same thing when converting to an expected, and then we could use expected in 1p code with StatusOr returned from 3p functions?

Abseil has accepted other backports, such as optional. I wonder if we can have Abseil grow an expected backport, in order to have StatusOr play well with it.