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

fix: fix RPC behavior when exception is thrown across boundary #301

Conversation

ngeojiajun-deriv
Copy link
Contributor

@ngeojiajun-deriv ngeojiajun-deriv commented Feb 13, 2024

Context

When an error is thrown by the listener, it is never deal properly by either client or the server side. Resulting to the following problems depending on the exception thrown.

  1. Any attempt to throw blessed objects (including Myriad::Exception implementers) would cause immediate crash which would cause the Future for processing the RPC calls cancelled.
  2. Any attempt to throw perl errors (ie. die) would cause caller to get undef as result instead of exceptions getting thrown.
  3. Exception handling code in RPC wrongly assume the presence of the does method, causing the (1) to happen

Changes

This PR contains breaking changes, please test it before merge

  1. Properly deal with non-serializable objects when dealing with exceptions
  2. Explicitly cast the error message to be always string
  3. call_rpc would rethrow all exceptions from the RPC server as Myriad::Exception::RPC::RemoteException
  4. Updated tests to cover these edge case

Copy link
Contributor

@tom-binary tom-binary left a comment

Choose a reason for hiding this comment

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

Main issue here is the class check - we shouldn't be doing string checks like this.

lib/Myriad/RPC/Client/Implementation/Memory.pm Outdated Show resolved Hide resolved
lib/Myriad/RPC/Client/Implementation/Memory.pm Outdated Show resolved Hide resolved
lib/Myriad/Service/Implementation.pm Outdated Show resolved Hide resolved
lib/Myriad/Service/Implementation.pm Outdated Show resolved Hide resolved
t/RPC/exceptions.t Show resolved Hide resolved
lib/Myriad/Service/Implementation.pm Outdated Show resolved Hide resolved
address reviews

Reintroduces the does shim

This reverts commit 132b176.

address review from tom
tom-binary added a commit to tom-binary/perl-Myriad that referenced this pull request Jul 22, 2024
We rely on `UNIVERSAL::DOES` here.

https://metacpan.org/pod/UNIVERSAL#$obj-%3EDOES(-ROLE-)

See also pull request deriv-com#301, which has a similar fix.
@tom-binary
Copy link
Contributor

Since there's not been any progress on this for a few months, I've merged #353 for basic exception support.

@tom-binary tom-binary closed this Oct 22, 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.

3 participants