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..0a32732c 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 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 7529984e..12339432 100644 --- a/lib/Myriad/RPC/Client/Implementation/Memory.pm +++ b/lib/Myriad/RPC/Client/Implementation/Memory.pm @@ -88,6 +88,12 @@ 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/) { @@ -95,7 +101,7 @@ async method call_rpc ($service, $method, %args) { } else { $e = Myriad::Exception::InternalError->new(reason => $e) unless blessed $e && $e->DOES('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 90a2ae5b..171eaaf1 100644 --- a/lib/Myriad/RPC/Client/Implementation/Redis.pm +++ b/lib/Myriad/RPC/Client/Implementation/Redis.pm @@ -97,6 +97,11 @@ async method call_rpc($service, $method, %args) { $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) { $log->warnf('Failed on RPC call - %s', $e); @@ -105,7 +110,7 @@ async method call_rpc($service, $method, %args) { } else { $e = Myriad::Exception::InternalError->new(reason => $e) unless blessed $e && $e->DOES('Myriad::Exception'); } - $pending->fail($e); + $pending->fail($e) unless $pending->is_ready; delete $pending_requests->{$message_id}; $e->throw(); } 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..1992dad1 100644 --- a/t/exception.t +++ b/t/exception.t @@ -18,6 +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'); + ok($ex->DOES('Myriad::Exception'), 'It inherits from Myriad::Exception role'); }; done_testing;