From 4ae4951cd5ebd8c9fbf377ef99f0170bd42b3ec8 Mon Sep 17 00:00:00 2001 From: ngeojiajun-deriv Date: Tue, 13 Feb 2024 12:55:49 +0800 Subject: [PATCH 1/2] fix: fix RPC behavior when exception is thrown accross boundary address reviews Reintroduces the does shim This reverts commit 132b176ec12f3162779f0b9bd6182cae89f7439d. address review from tom --- .gitignore | 2 + lib/Myriad/Exception/Base.pm | 32 +++++++- lib/Myriad/RPC.pm | 10 +++ .../RPC/Client/Implementation/Memory.pm | 10 ++- lib/Myriad/RPC/Client/Implementation/Redis.pm | 9 +- lib/Myriad/Service/Implementation.pm | 8 +- t/RPC/exceptions.t | 82 +++++++++++++++++++ t/exception.t | 3 + 8 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 t/RPC/exceptions.t diff --git a/.gitignore b/.gitignore index df95baf4..ec2c229b 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ nytprof.out # Dizt::Zilla /.build/ +/deps/ # Module::Build _build/ @@ -33,6 +34,7 @@ inc/ /MANIFEST.bak /pm_to_blib /*.zip +/Myriad-*/ # IDEs ( Some people !) .idea diff --git a/lib/Myriad/Exception/Base.pm b/lib/Myriad/Exception/Base.pm index 9882b1f4..dd605cfd 100644 --- a/lib/Myriad/Exception/Base.pm +++ b/lib/Myriad/Exception/Base.pm @@ -27,7 +27,15 @@ use overload '""' => sub { shift->as_string }, bool => sub { 1 }, fallback => 1; sub new { my ($class, %args) = @_; - bless \%args, $class + # Force the reason to be a valid string + # or it would cause issue with strongly typed clients. + # Moreover, attempt to use blessed object here would crash the listener + if ($args{reason}) { + my $reason = "$args{reason}"; + $reason =~ s/(\s+)$//; + $args{reason} = $reason; + } + bless \%args, $class; } =head2 reason @@ -46,6 +54,28 @@ Returns the exception message as a string. sub as_string { shift->message } +=head2 does + +Check the role ownership of the objects, added here to maintain compatibility with Object::Pad exports. +This is needed as some part of Myriad code assume that the objects created by Myriad exceptions do exports does, +which in turn to be absent. + +=cut + +sub does { + use Object::Pad::MOP::Class qw(:experimental); + my ($self, $role) = @_; + + return 0 unless $role and ref $self; + + my $klass = ref $self; + my $klass_ptr = Object::Pad::MOP::Class->try_for_class($klass); + return 0 unless defined $klass_ptr; + + my %roles = map {($_->name) => 1} $klass_ptr->all_roles; + return $roles{$role} // 0; +} + 1; =head1 AUTHOR diff --git a/lib/Myriad/RPC.pm b/lib/Myriad/RPC.pm index 907e92f1..1bd92466 100644 --- a/lib/Myriad/RPC.pm +++ b/lib/Myriad/RPC.pm @@ -81,6 +81,16 @@ declare_exception UnknownTransport => ( message => 'Unknown transport' ); +=head2 RemoteException + +An exception has been thrown from remote service + +=cut + +declare_exception RemoteException => ( + message => 'The RPC method failed at server side' +); + =head1 METHODS =cut diff --git a/lib/Myriad/RPC/Client/Implementation/Memory.pm b/lib/Myriad/RPC/Client/Implementation/Memory.pm index a015596b..71a144d5 100644 --- a/lib/Myriad/RPC/Client/Implementation/Memory.pm +++ b/lib/Myriad/RPC/Client/Implementation/Memory.pm @@ -88,14 +88,20 @@ async method call_rpc ($service, $method, %args) { $self->loop->timeout_future(at => $deadline), $pending ); + + unless (exists $message->response->{response}) { + my $reason = $message->response->{error}{message} // "Unknown"; + Myriad::Exception::RPC::RemoteException->throw(reason => "Remote exception is thrown: $reason"); + } + return $message->response->{response}; } catch ($e) { if ($e =~ /Timeout/) { $e = Myriad::Exception::RPC::Timeout->new(reason => 'deadline is due'); } else { - $e = Myriad::Exception::InternalError->new(reason => $e) unless blessed $e && $e->does('Myriad::Exception'); + $e = Myriad::Exception::InternalError->new(reason => $e) unless Myriad::Exception::Base::does($e, 'Myriad::Exception'); } - $pending->fail($e); + $pending->fail($e) unless $pending->is_ready; delete $pending_requests->{$message_id}; $e->throw(); } diff --git a/lib/Myriad/RPC/Client/Implementation/Redis.pm b/lib/Myriad/RPC/Client/Implementation/Redis.pm index 4b875cf8..d3533c26 100644 --- a/lib/Myriad/RPC/Client/Implementation/Redis.pm +++ b/lib/Myriad/RPC/Client/Implementation/Redis.pm @@ -95,14 +95,19 @@ async method call_rpc($service, $method, %args) { # The subscription loop will parse the message for us my $message = await Future->wait_any($self->loop->timeout_future(after => $timeout), $pending); + unless (exists $message->response->{response}) { + my $reason = $message->response->{error}{message} // "Unknown"; + Myriad::Exception::RPC::RemoteException->throw(reason => "Remote exception is thrown: $reason"); + } + return $message->response->{response}; } catch ($e) { if ($e =~ /Timeout/) { $e = Myriad::Exception::RPC::Timeout->new(reason => 'deadline is due'); } else { - $e = Myriad::Exception::InternalError->new(reason => $e) unless blessed $e && $e->does('Myriad::Exception'); + $e = Myriad::Exception::InternalError->new(reason => $e) unless Myriad::Exception::Base::does($e, 'Myriad::Exception'); } - $pending->fail($e); + $pending->fail($e) unless $pending->is_ready; delete $pending_requests->{$message_id}; $e->throw(); } diff --git a/lib/Myriad/Service/Implementation.pm b/lib/Myriad/Service/Implementation.pm index cce99fd5..cc15641e 100644 --- a/lib/Myriad/Service/Implementation.pm +++ b/lib/Myriad/Service/Implementation.pm @@ -358,9 +358,11 @@ async method load () { return await $self->rpc->reply_success($service_name, $message, $response); } } catch ($e) { - $e = Myriad::Exception::InternalError->new( - reason => $e - ) unless blessed($e) and $e->does('Myriad::Exception'); + # Pass through the myriad exception, use indirect exports from Myriad::Exception::Base + # for simplicity + unless (blessed $e and Myriad::Exception::Base::does($e, 'Myriad::Exception')) { + $e = Myriad::Exception::InternalError->new(reason => $e); + } if(USE_OPENTELEMETRY) { $span->record_exception($e); $span->set_status( diff --git a/t/RPC/exceptions.t b/t/RPC/exceptions.t new file mode 100644 index 00000000..52137a73 --- /dev/null +++ b/t/RPC/exceptions.t @@ -0,0 +1,82 @@ +use strict; +use warnings; + +use Test::More; +use Test::Deep; +use Test::Fatal; +use Test::Myriad; + +use Future; +use Future::AsyncAwait; +use Object::Pad qw(:experimental); +use Myriad::Exception::InternalError; + +my ($broken_services); + +# Example of some broken behavior that implementations can have +package Test::BrokenServices { + use Myriad::Service; + async method normal : RPC (%args) { + return {pong => 1}; + } + async method normal_dead : RPC (%args) { + die "I am died"; + } + async method hash_dead : RPC (%args) { + die {reason => "I am died"}; + } + async method blessed_dead : RPC (%args) { + die bless {reason => "I am died"}, "Test::Exceptions"; + } + async method bubbled_dead : RPC (%args) { + # Fake it as something is wrong with Myriad itself + Myriad::Exception::InternalError->throw(); + } +} + +BEGIN { + $broken_services = Test::Myriad->add_service(service => 'Test::BrokenServices'); +} + + +await Test::Myriad->ready(); + +subtest 'Normal RPC calls must succeeded as usual' => sub { + my $resposne = $broken_services->call_rpc('normal')->get; + cmp_deeply($resposne, {pong => 1}); +}; + +subtest 'Dead RPC calls must have its exceptions bubbled up to caller' => sub { + my $failure = exception { + $broken_services->call_rpc('normal_dead')->get; + }; + is(ref $failure, "Myriad::Exception::RPC::RemoteException", "Correct error received by caller"); + like($failure->reason, qr/reason=I am died/, "Correct error message"); +}; + +subtest 'HASHREFs must not cross RPC boundary!' => sub { + my $failure = exception { + $broken_services->call_rpc('hash_dead')->get; + }; + is(ref $failure, "Myriad::Exception::RPC::RemoteException", "Correct error received by caller"); + like($failure->reason, qr/reason=HASH\(0x[a-zA-Z0-9]+\)/, "Correct error message"); +}; + +subtest 'blessed HASHREFs must not cross RPC boundary!' => sub { + my $failure = exception { + $broken_services->call_rpc('blessed_dead')->get; + }; + is(ref $failure, "Myriad::Exception::RPC::RemoteException", "Correct error received by caller"); + like($failure->reason, qr/reason=Test::Exceptions=HASH\(0x[a-zA-Z0-9]+\)/, "Correct error message"); +}; + +subtest 'Internal Myriad errors on remote must not appear as local Myriad error!' => sub { + my $failure = exception { + $broken_services->call_rpc('bubbled_dead')->get; + }; + is(ref $failure, "Myriad::Exception::RPC::RemoteException", "Correct error received by caller"); + is($failure->reason, 'Remote exception is thrown: Internal error (category=internal)', "Correct error message"); +}; + +done_testing(); + diff --git a/t/exception.t b/t/exception.t index 15073de6..31132b39 100644 --- a/t/exception.t +++ b/t/exception.t @@ -18,6 +18,9 @@ subtest 'can declare exceptions' => sub { is($ex->message, 'this is a message (category=some_category)', 'message is correct'); is($ex->category, 'some_category', 'category is correct'); is("$ex", 'this is a message (category=some_category)', 'stringifies too'); + # Some of the Myriad RPC code expect the does method is present but it doesn't in old version + # This would enforce it + ok($ex->does('Myriad::Exception'), 'It inherits from Myriad::Exception role'); }; done_testing; From a184eb6c7f71b365ead64f9cfafdc96d1e4879e1 Mon Sep 17 00:00:00 2001 From: ngeojiajun-deriv Date: Thu, 29 Feb 2024 11:22:04 +0800 Subject: [PATCH 2/2] use DOES instead of does --- lib/Myriad/Exception/Base.pm | 22 ------------------- .../RPC/Client/Implementation/Memory.pm | 2 +- lib/Myriad/RPC/Client/Implementation/Redis.pm | 2 +- lib/Myriad/Service/Implementation.pm | 6 +---- t/exception.t | 4 +--- 5 files changed, 4 insertions(+), 32 deletions(-) diff --git a/lib/Myriad/Exception/Base.pm b/lib/Myriad/Exception/Base.pm index dd605cfd..0a32732c 100644 --- a/lib/Myriad/Exception/Base.pm +++ b/lib/Myriad/Exception/Base.pm @@ -54,28 +54,6 @@ Returns the exception message as a string. sub as_string { shift->message } -=head2 does - -Check the role ownership of the objects, added here to maintain compatibility with Object::Pad exports. -This is needed as some part of Myriad code assume that the objects created by Myriad exceptions do exports does, -which in turn to be absent. - -=cut - -sub does { - use Object::Pad::MOP::Class qw(:experimental); - my ($self, $role) = @_; - - return 0 unless $role and ref $self; - - my $klass = ref $self; - my $klass_ptr = Object::Pad::MOP::Class->try_for_class($klass); - return 0 unless defined $klass_ptr; - - my %roles = map {($_->name) => 1} $klass_ptr->all_roles; - return $roles{$role} // 0; -} - 1; =head1 AUTHOR diff --git a/lib/Myriad/RPC/Client/Implementation/Memory.pm b/lib/Myriad/RPC/Client/Implementation/Memory.pm index 71a144d5..f9de8a17 100644 --- a/lib/Myriad/RPC/Client/Implementation/Memory.pm +++ b/lib/Myriad/RPC/Client/Implementation/Memory.pm @@ -99,7 +99,7 @@ async method call_rpc ($service, $method, %args) { if ($e =~ /Timeout/) { $e = Myriad::Exception::RPC::Timeout->new(reason => 'deadline is due'); } else { - $e = Myriad::Exception::InternalError->new(reason => $e) unless Myriad::Exception::Base::does($e, 'Myriad::Exception'); + $e = Myriad::Exception::InternalError->new(reason => $e) unless blessed $e and $e->DOES('Myriad::Exception'); } $pending->fail($e) unless $pending->is_ready; delete $pending_requests->{$message_id}; diff --git a/lib/Myriad/RPC/Client/Implementation/Redis.pm b/lib/Myriad/RPC/Client/Implementation/Redis.pm index d3533c26..d906650c 100644 --- a/lib/Myriad/RPC/Client/Implementation/Redis.pm +++ b/lib/Myriad/RPC/Client/Implementation/Redis.pm @@ -105,7 +105,7 @@ async method call_rpc($service, $method, %args) { if ($e =~ /Timeout/) { $e = Myriad::Exception::RPC::Timeout->new(reason => 'deadline is due'); } else { - $e = Myriad::Exception::InternalError->new(reason => $e) unless Myriad::Exception::Base::does($e, 'Myriad::Exception'); + $e = Myriad::Exception::InternalError->new(reason => $e) unless blessed $e and $e->DOES('Myriad::Exception'); } $pending->fail($e) unless $pending->is_ready; delete $pending_requests->{$message_id}; diff --git a/lib/Myriad/Service/Implementation.pm b/lib/Myriad/Service/Implementation.pm index cc15641e..6846271b 100644 --- a/lib/Myriad/Service/Implementation.pm +++ b/lib/Myriad/Service/Implementation.pm @@ -358,11 +358,7 @@ async method load () { return await $self->rpc->reply_success($service_name, $message, $response); } } catch ($e) { - # Pass through the myriad exception, use indirect exports from Myriad::Exception::Base - # for simplicity - unless (blessed $e and Myriad::Exception::Base::does($e, 'Myriad::Exception')) { - $e = Myriad::Exception::InternalError->new(reason => $e); - } + $e = Myriad::Exception::InternalError->new(reason => $e) unless blessed $e and $e->DOES('Myriad::Exception'); if(USE_OPENTELEMETRY) { $span->record_exception($e); $span->set_status( diff --git a/t/exception.t b/t/exception.t index 31132b39..1992dad1 100644 --- a/t/exception.t +++ b/t/exception.t @@ -18,9 +18,7 @@ subtest 'can declare exceptions' => sub { is($ex->message, 'this is a message (category=some_category)', 'message is correct'); is($ex->category, 'some_category', 'category is correct'); is("$ex", 'this is a message (category=some_category)', 'stringifies too'); - # Some of the Myriad RPC code expect the does method is present but it doesn't in old version - # This would enforce it - ok($ex->does('Myriad::Exception'), 'It inherits from Myriad::Exception role'); + ok($ex->DOES('Myriad::Exception'), 'It inherits from Myriad::Exception role'); }; done_testing;