Skip to content

Conversation

@nerfZael
Copy link
Contributor

@nerfZael nerfZael commented May 16, 2023

The goal of this PR is to decouple abort handling and URI from the wrapper runtime.

  • URI has been removed from the wrapper.invoke function (it was used just for the abort message)
  • Abort handler can now be provided to the wrapper.invoke function which will get called in case of an unrecoverable exception
    The abort handler allows the client to handle the abort logic instead of leaving it to the individual wrapper runtime implementations. This gives more power to the client, it allows it to standardize abort logic across all wrappers and, in the future, it can allow users to customize the abort logic (by providing a custom abort handler).

Other notable changes:

  • Decode result errors are now MsgpackError instead of InvokeError
  • Failing to resolve wrapper (uri not found) is now a ResolutionError instead of InvokeError
  • InvokeError now contains the URI and method
  • Removed PluginError::ModuleError and PluginError::SubinvocationError as they are not being used
  • Removed URI, method and args from PluginError::InvocationError. URI and method can/will be included in the parent InvokeError, while args I feel are not a good thing to output by default because they could contain sensitive data. That could be changed by the user in the future when we allow them to provide a custom abort handler (Or if the user "inherits" the PolywrapClient and overrides the invoke_wrapper_raw method).

@nerfZael nerfZael changed the title Removal of URI from wrapper invoke and abort handler implementation Abort handler implementation and removal of URI from wrapper invoke May 17, 2023
@nerfZael nerfZael changed the title Abort handler implementation and removal of URI from wrapper invoke Abort handler implementation and removal of URI from wrapper.invoke May 17, 2023
@nerfZael nerfZael requested review from cbrzn and namesty May 17, 2023 10:56
@nerfZael nerfZael marked this pull request as ready for review May 17, 2023 10:56
@nerfZael nerfZael changed the base branch from nerfzael-resolution-context-refactor to main May 17, 2023 16:03
@namesty namesty merged commit 8100514 into main May 18, 2023
@krisbitney
Copy link
Contributor

@nerfZael @namesty What happens if the user doesn't panic! in the abortHandler?

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