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

rework errors #42

Merged
merged 6 commits into from
Aug 7, 2024
Merged

rework errors #42

merged 6 commits into from
Aug 7, 2024

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Aug 6, 2024

Issue #, if available:
#13

Description of changes:

Refactor our error representation into a single struct with an ErrorKind.

Rationale

Smithy errors are designed based on whether they are “informative” or “actionable” (see RFC-0022).

Below are the actual modeled SDK errors for the operations the S3 transfer manager is expected to invoke:

pub enum ListObjectsV2Error {
    NoSuchBucket(NoSuchBucket),
    Unhandled(Unhandled),
}

pub enum HeadObjectError {
    NotFound(NotFound),
    Unhandled(Unhandled),
}

pub enum GetObjectError {
    InvalidObjectState(InvalidObjectState),
    NoSuchKey(NoSuchKey),
    Unhandled(Unhandled),
}

pub enum PutObjectError {
    Unhandled(Unhandled),
}

pub enum UploadPartError {
    Unhandled(Unhandled),
}

pub enum CreateMultipartUploadError {
    Unhandled(Unhandled),
}

pub enum CompleteMultipartUploadError {
    Unhandled(Unhandled),
}

pub enum AbortMultipartUploadError {
    NoSuchUpload(NoSuchUpload),
    Unhandled(Unhandled),
}

There are not many variants here worth propagating up or that a user would find actionable. As such it makes little sense to expose them directly. The exception to this would be to access the raw HTTP response and/or metadata (e.g. request ID) but we should be able to capture that and expose it more conveniently for users without resorting to exposing the raw error.

The rest of our errors are more informative or bugs (e.g. invalid user input, runtime errors from poisoned mutex, join errors, etc).

This lead us to settle on a single struct with ErrorKind enum similar to std::io::Error.

Open Questions/Future

  1. Error representation for when an error is encountered while already processing an error (e.g. upload of a single part fails; failure policy is to abort, we invoke AbortMutlipartUpload and that fails for some reason.) Do we make the secondary error available somehow or somehow expose that "cleanup" failed?
  2. Metadata and raw HTTP response headers
    • We should be able to capture this and expose it fairly easily from the underlying SdkError

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aajtodd aajtodd requested a review from a team as a code owner August 6, 2024 17:03
Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

I like how this rework of errors came out!

Error representation for when an error is encountered while already processing an error

Yeah, maybe some sort of chain of errors so customers can trace the reasons of failures, but I'm not sure. Will certainly iterate on this.

Comment on lines 75 to 78
/// Returns the corresponding [`ErrorKind`] for this error.
pub fn kind(&self) -> ErrorKind {
self.kind.clone()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In smithy-rs, we have two patterns for the signature:

    pub fn kind(&self) -> ErrorKind
    pub fn kind(&self) -> &ErrorKind

I'd prefer the latter since it leaves the option to callers whether they want to clone ErrorKind or not, whereas the former always pays the cost of cloning the kind even when callers just need reference at the end of the day (cloning the kind could become more expensive depending on a new variant in the future). I'll leave it up to you, though.

@@ -493,7 +493,6 @@ impl UploadOutputBuilder {
}

/// Consumes the builder and constructs a [`UploadOutput`]
// FIXME(aws-sdk-rust#1159): replace BuildError with our own type?
pub fn build(self) -> Result<UploadOutput, ::aws_smithy_types::error::operation::BuildError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point, we probably want to re-export of BuildError in this library and use it here, reducing exposure of types from a different crate.

fn from(value: aws_sdk_s3::error::SdkError<E, R>) -> Self {
// TODO - extract request id/metadata
let kind = match value.code() {
Some("NotFound") | Some("NoSuchKey") | Some("NoSuchUpload") | Some("NoSuchBucket") => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Some("NotFound") | Some("NoSuchKey") | Some("NoSuchUpload") | Some("NoSuchBucket") => {
Some("NotFound" | "NoSuchKey" | "NoSuchUpload" | "NoSuchBucket") => {

I think this works with MSRV >= 1.76

Comment on lines +15 to +16
/// NOTE: Use [`aws_smithy_types::error::display::DisplayErrorContext`] or similar to display
/// the entire error cause/source chain.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish error reporters were farther along so we could be specific here. Is noting "or similar" helpful? Will people understand what that means?

Comment on lines +23 to +46
/// General categories of transfer errors.
#[derive(Debug, Clone)]
#[non_exhaustive]
pub enum ErrorKind {
/// Operation input validation issues
InputInvalid,

/// I/O errors
IOError,

/// Some kind of internal runtime issue (e.g. task failure, poisoned mutex, etc)
RuntimeError,

/// Object discovery failed
ObjectNotDiscoverable,

/// Failed to upload or download a chunk of an object
ChunkFailed,

/// Resource not found (e.g. bucket, key, multipart upload ID not found)
NotFound,

/// child operation failed (e.g. download of a single object as part of downloading all objects from a bucket)
ChildOperationFailed,
Copy link
Contributor

Choose a reason for hiding this comment

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

With the way that these are defined, we can't add context to them later. That may be fine. Otherwise, we should define them like so:

/// General categories of transfer errors.
#[derive(Debug, Clone)]
#[non_exhaustive]
pub enum ErrorKind {
    /// Operation input validation issues
    InputInvalid(InputInvalid),
}

pub struct InputInvalid {} 

@aajtodd aajtodd merged commit b96966c into main Aug 7, 2024
14 checks passed
@aajtodd aajtodd deleted the atodd/errors-redux branch August 7, 2024 18:14
Copy link
Contributor

@graebm graebm left a comment

Choose a reason for hiding this comment

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

👍

graebm pushed a commit that referenced this pull request Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants