Skip to content

Conversation

@zaporter-work
Copy link
Contributor

@zaporter-work zaporter-work commented May 11, 2023

Ticket: https://viam.atlassian.net/browse/RSDK-2758

This provides a base ViamException class and a few derived classes. The purpose of these exceptions are threefold:

  • To provide a base to build on if we wish to pursue more advanced error creation in the future.
  • To help users easily understand if an error came from our code or not.
  • To facilitate error-catching and recovery behavior (ex: a DuplicateResourceException might be handled differently than a PermissionDeniedException)

This PR also creates ViamErrorCodes which are 1 to 1 compatible with the exceptions and can be used interchangeably (though conversion will lose the message inside the exception).

While working on this, I experimented with three main design options:
1)

   template <typename... Args>
    ViamException(Args const&... what)
        : std::runtime_error("ViamException: " + concat_varargs(what...)){};

I really liked this approach because it let me write very clean constructor calls. However this produced awful error messages (SFINAE was overkill for concat_varargs), and it required almost everything to live in the header files.

#define THROW_EXCEPTION(e_type, msg) throw e_type(__FILE__, __LINE__, __func__, msg)

// ex: THROW_EXCEPTION(DuplicateResourceException, "can't have two camera servers ...");

I really liked this approach because it gave very nice error messages; however, I am hesitant to suggest that we use macros. (we might also need to do some macro magic so that the full path of __FILE__ doesn't end up in the binary)

The simple

ViamException(const std::string& msg) : std::runtime_exception("ViamException: " + msg) {}

Ultimately, I landed on number 3 because number 1 didn't end up providing as many benefits as I wanted and we can do number 2 later if we want to give more detailed error messages.

Important Notes:

  1. Some of the exception types are not being used right now. I still created them because they may be useful in the future (and because they line up roughly with the python sdk)
  2. Conversion to and from error codes is a bit awkward. Wasn't sure about the best way to do this.

@zaporter-work zaporter-work requested review from acmorrow and stuqdog May 11, 2023 06:45
@zaporter-work zaporter-work requested a review from a team as a code owner May 11, 2023 06:45
@zaporter-work zaporter-work requested review from clintpurser and removed request for a team and clintpurser May 11, 2023 06:45
Comment on lines -21 to -25
try {
this->fix_api();
} catch (std::string err) { // NOLINT
throw err;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this do something that I don't understand?

Copy link
Member

Choose a reason for hiding this comment

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

Probably not! Looks like the try/catch is pretty much meaningless there, and catching a str is just an artifact of poor initial design decisions. Makes sense to remove.

Comment on lines -69 to -71
// TODO(RSDK-1742) Replace throwing of strings with throwing of
// runtime_error
throw "Unable to establish connecting path!";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket was marked as completed

Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

This is great, thanks for getting this done! LGTM, % questions/nits.

@zaporter-work
Copy link
Contributor Author

zaporter-work commented Jul 11, 2023

Closing due to 2 months of inactivity. Happy to talk about or reopen this if there is renewed interest.

@zaporter-work zaporter-work deleted the zp/exception branch January 25, 2024 17:50
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.

2 participants