From 8661a43f784e6c2689466355dd5a0bdab4567346 Mon Sep 17 00:00:00 2001
From: Tim Emiola <temiola@google.com>
Date: Fri, 17 Apr 2015 13:29:59 -0700
Subject: [PATCH] Propagate metadata in BadStatus
- allow BadStatus to contain metadata that's populated by keyword args
- on servers, convert raised BadStatus metadata to trailers
- on clients, convert trailers to BadStatus metadata when raising BadStatus
---
src/ruby/.rubocop_todo.yml | 8 +--
src/ruby/lib/grpc/generic/active_call.rb | 13 +++-
src/ruby/lib/grpc/generic/rpc_desc.rb | 10 +--
src/ruby/spec/generic/rpc_desc_spec.rb | 88 +++++++++++-------------
src/ruby/spec/generic/rpc_server_spec.rb | 56 ++++++++++++++-
5 files changed, 113 insertions(+), 62 deletions(-)
diff --git a/src/ruby/.rubocop_todo.yml b/src/ruby/.rubocop_todo.yml
index 02136a81a9..6b8f336055 100644
--- a/src/ruby/.rubocop_todo.yml
+++ b/src/ruby/.rubocop_todo.yml
@@ -1,18 +1,18 @@
# This configuration was generated by `rubocop --auto-gen-config`
-# on 2015-04-16 12:30:09 -0700 using RuboCop version 0.30.0.
+# on 2015-04-17 12:36:26 -0700 using RuboCop version 0.30.0.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
# versions of RuboCop, may require this file to be generated again.
-# Offense count: 34
+# Offense count: 30
Metrics/AbcSize:
- Max: 36
+ Max: 37
# Offense count: 3
# Configuration parameters: CountComments.
Metrics/ClassLength:
- Max: 185
+ Max: 179
# Offense count: 35
# Configuration parameters: CountComments.
diff --git a/src/ruby/lib/grpc/generic/active_call.rb b/src/ruby/lib/grpc/generic/active_call.rb
index 8d63de4145..274372e4d5 100644
--- a/src/ruby/lib/grpc/generic/active_call.rb
+++ b/src/ruby/lib/grpc/generic/active_call.rb
@@ -39,7 +39,10 @@ class Struct
return nil if status.nil?
fail GRPC::Cancelled if status.code == GRPC::Core::StatusCodes::CANCELLED
if status.code != GRPC::Core::StatusCodes::OK
- fail GRPC::BadStatus.new(status.code, status.details)
+ # raise BadStatus, propagating the metadata if present.
+ md = status.metadata
+ with_sym_keys = Hash[md.each_pair.collect { |x, y| [x.to_sym, y] }]
+ fail GRPC::BadStatus.new(status.code, status.details, **with_sym_keys)
end
status
end
@@ -192,9 +195,13 @@ module GRPC
# @param details [String] details
# @param assert_finished [true, false] when true(default), waits for
# FINISHED.
- def send_status(code = OK, details = '', assert_finished = false)
+ #
+ # == Keyword Arguments ==
+ # any keyword arguments are treated as metadata to be sent to the server
+ # if a keyword value is a list, multiple metadata for it's key are sent
+ def send_status(code = OK, details = '', assert_finished = false, **kw)
ops = {
- SEND_STATUS_FROM_SERVER => Struct::Status.new(code, details)
+ SEND_STATUS_FROM_SERVER => Struct::Status.new(code, details, kw)
}
ops[RECV_CLOSE_ON_SERVER] = nil if assert_finished
@call.run_batch(@cq, self, INFINITE_FUTURE, ops)
diff --git a/src/ruby/lib/grpc/generic/rpc_desc.rb b/src/ruby/lib/grpc/generic/rpc_desc.rb
index 3e48b8e51d..22b80d8938 100644
--- a/src/ruby/lib/grpc/generic/rpc_desc.rb
+++ b/src/ruby/lib/grpc/generic/rpc_desc.rb
@@ -82,10 +82,10 @@ module GRPC
end
send_status(active_call, OK, 'OK')
rescue BadStatus => e
- # this is raised by handlers that want GRPC to send an application
- # error code and detail message.
+ # this is raised by handlers that want GRPC to send an application error
+ # code and detail message and some additional app-specific metadata.
logger.debug("app err: #{active_call}, status:#{e.code}:#{e.details}")
- send_status(active_call, e.code, e.details)
+ send_status(active_call, e.code, e.details, **e.metadata)
rescue Core::CallError => e
# This is raised by GRPC internals but should rarely, if ever happen.
# Log it, but don't notify the other endpoint..
@@ -135,9 +135,9 @@ module GRPC
"##{mth.name}: bad arg count; got:#{mth.arity}, want:#{want}, #{msg}"
end
- def send_status(active_client, code, details)
+ def send_status(active_client, code, details, **kw)
details = 'Not sure why' if details.nil?
- active_client.send_status(code, details, code == OK)
+ active_client.send_status(code, details, code == OK, **kw)
rescue StandardError => e
logger.warn("Could not send status #{code}:#{details}")
logger.warn(e)
diff --git a/src/ruby/spec/generic/rpc_desc_spec.rb b/src/ruby/spec/generic/rpc_desc_spec.rb
index a68299465c..e5d05c8b9b 100644
--- a/src/ruby/spec/generic/rpc_desc_spec.rb
+++ b/src/ruby/spec/generic/rpc_desc_spec.rb
@@ -52,41 +52,47 @@ describe GRPC::RpcDesc do
@ok_response = Object.new
end
+ shared_examples 'it handles errors' do
+ it 'sends the specified status if BadStatus is raised' do
+ expect(@call).to receive(:remote_read).once.and_return(Object.new)
+ expect(@call).to receive(:send_status).once.with(@bs_code, 'NOK', false,
+ {})
+ this_desc.run_server_method(@call, method(:bad_status))
+ end
+
+ it 'sends status UNKNOWN if other StandardErrors are raised' do
+ expect(@call).to receive(:remote_read).once.and_return(Object.new)
+ expect(@call).to receive(:send_status) .once.with(UNKNOWN, @no_reason,
+ false, {})
+ this_desc.run_server_method(@call, method(:other_error))
+ end
+
+ it 'absorbs CallError with no further action' do
+ expect(@call).to receive(:remote_read).once.and_raise(CallError)
+ blk = proc do
+ this_desc.run_server_method(@call, method(:fake_reqresp))
+ end
+ expect(&blk).to_not raise_error
+ end
+ end
+
describe '#run_server_method' do
describe 'for request responses' do
+ let(:this_desc) { @request_response }
before(:each) do
@call = double('active_call')
allow(@call).to receive(:single_req_view).and_return(@call)
allow(@call).to receive(:gc)
end
- it 'sends the specified status if BadStatus is raised' do
- expect(@call).to receive(:remote_read).once.and_return(Object.new)
- expect(@call).to receive(:send_status).once.with(@bs_code, 'NOK', false)
- @request_response.run_server_method(@call, method(:bad_status))
- end
-
- it 'sends status UNKNOWN if other StandardErrors are raised' do
- expect(@call).to receive(:remote_read).once.and_return(Object.new)
- expect(@call).to receive(:send_status) .once.with(UNKNOWN, @no_reason,
- false)
- @request_response.run_server_method(@call, method(:other_error))
- end
-
- it 'absorbs CallError with no further action' do
- expect(@call).to receive(:remote_read).once.and_raise(CallError)
- blk = proc do
- @request_response.run_server_method(@call, method(:fake_reqresp))
- end
- expect(&blk).to_not raise_error
- end
+ it_behaves_like 'it handles errors'
it 'sends a response and closes the stream if there no errors' do
req = Object.new
expect(@call).to receive(:remote_read).once.and_return(req)
expect(@call).to receive(:remote_send).once.with(@ok_response)
- expect(@call).to receive(:send_status).once.with(OK, 'OK', true)
- @request_response.run_server_method(@call, method(:fake_reqresp))
+ expect(@call).to receive(:send_status).once.with(OK, 'OK', true, {})
+ this_desc.run_server_method(@call, method(:fake_reqresp))
end
end
@@ -98,13 +104,14 @@ describe GRPC::RpcDesc do
end
it 'sends the specified status if BadStatus is raised' do
- expect(@call).to receive(:send_status).once.with(@bs_code, 'NOK', false)
+ expect(@call).to receive(:send_status).once.with(@bs_code, 'NOK', false,
+ {})
@client_streamer.run_server_method(@call, method(:bad_status_alt))
end
it 'sends status UNKNOWN if other StandardErrors are raised' do
expect(@call).to receive(:send_status) .once.with(UNKNOWN, @no_reason,
- false)
+ false, {})
@client_streamer.run_server_method(@call, method(:other_error_alt))
end
@@ -118,44 +125,26 @@ describe GRPC::RpcDesc do
it 'sends a response and closes the stream if there no errors' do
expect(@call).to receive(:remote_send).once.with(@ok_response)
- expect(@call).to receive(:send_status).once.with(OK, 'OK', true)
+ expect(@call).to receive(:send_status).once.with(OK, 'OK', true, {})
@client_streamer.run_server_method(@call, method(:fake_clstream))
end
end
describe 'for server streaming' do
+ let(:this_desc) { @request_response }
before(:each) do
@call = double('active_call')
allow(@call).to receive(:single_req_view).and_return(@call)
allow(@call).to receive(:gc)
end
- it 'sends the specified status if BadStatus is raised' do
- expect(@call).to receive(:remote_read).once.and_return(Object.new)
- expect(@call).to receive(:send_status).once.with(@bs_code, 'NOK', false)
- @server_streamer.run_server_method(@call, method(:bad_status))
- end
-
- it 'sends status UNKNOWN if other StandardErrors are raised' do
- expect(@call).to receive(:remote_read).once.and_return(Object.new)
- expect(@call).to receive(:send_status) .once.with(UNKNOWN, @no_reason,
- false)
- @server_streamer.run_server_method(@call, method(:other_error))
- end
-
- it 'absorbs CallError with no further action' do
- expect(@call).to receive(:remote_read).once.and_raise(CallError)
- blk = proc do
- @server_streamer.run_server_method(@call, method(:fake_svstream))
- end
- expect(&blk).to_not raise_error
- end
+ it_behaves_like 'it handles errors'
it 'sends a response and closes the stream if there no errors' do
req = Object.new
expect(@call).to receive(:remote_read).once.and_return(req)
expect(@call).to receive(:remote_send).twice.with(@ok_response)
- expect(@call).to receive(:send_status).once.with(OK, 'OK', true)
+ expect(@call).to receive(:send_status).once.with(OK, 'OK', true, {})
@server_streamer.run_server_method(@call, method(:fake_svstream))
end
end
@@ -172,20 +161,21 @@ describe GRPC::RpcDesc do
it 'sends the specified status if BadStatus is raised' do
e = GRPC::BadStatus.new(@bs_code, 'NOK')
expect(@call).to receive(:run_server_bidi).and_raise(e)
- expect(@call).to receive(:send_status).once.with(@bs_code, 'NOK', false)
+ expect(@call).to receive(:send_status).once.with(@bs_code, 'NOK', false,
+ {})
@bidi_streamer.run_server_method(@call, method(:bad_status_alt))
end
it 'sends status UNKNOWN if other StandardErrors are raised' do
expect(@call).to receive(:run_server_bidi).and_raise(StandardError)
expect(@call).to receive(:send_status).once.with(UNKNOWN, @no_reason,
- false)
+ false, {})
@bidi_streamer.run_server_method(@call, method(:other_error_alt))
end
it 'closes the stream if there no errors' do
expect(@call).to receive(:run_server_bidi)
- expect(@call).to receive(:send_status).once.with(OK, 'OK', true)
+ expect(@call).to receive(:send_status).once.with(OK, 'OK', true, {})
@bidi_streamer.run_server_method(@call, method(:fake_bidistream))
end
end
diff --git a/src/ruby/spec/generic/rpc_server_spec.rb b/src/ruby/spec/generic/rpc_server_spec.rb
index 202576c673..f1d95be553 100644
--- a/src/ruby/spec/generic/rpc_server_spec.rb
+++ b/src/ruby/spec/generic/rpc_server_spec.rb
@@ -58,7 +58,7 @@ class NoRpcImplementation
rpc :an_rpc, EchoMsg, EchoMsg
end
-# A test service with an implementation.
+# A test service with an echo implementation.
class EchoService
include GRPC::GenericService
rpc :an_rpc, EchoMsg, EchoMsg
@@ -77,6 +77,25 @@ end
EchoStub = EchoService.rpc_stub_class
+# A test service with an implementation that fails with BadStatus
+class FailingService
+ include GRPC::GenericService
+ rpc :an_rpc, EchoMsg, EchoMsg
+ attr_reader :details, :code, :md
+
+ def initialize(_default_var = 'ignored')
+ @details = 'app error'
+ @code = 101
+ @md = { failed_method: 'an_rpc' }
+ end
+
+ def an_rpc(_req, _call)
+ fail GRPC::BadStatus.new(@code, @details, **@md)
+ end
+end
+
+FailingStub = FailingService.rpc_stub_class
+
# A slow test service.
class SlowService
include GRPC::GenericService
@@ -514,5 +533,40 @@ describe GRPC::RpcServer do
t.join
end
end
+
+ context 'with metadata on failing' do
+ before(:each) do
+ server_opts = {
+ server_override: @server,
+ completion_queue_override: @server_queue,
+ poll_period: 1
+ }
+ @srv = RpcServer.new(**server_opts)
+ end
+
+ it 'should send receive metadata failed response', server: true do
+ service = FailingService.new
+ @srv.handle(service)
+ t = Thread.new { @srv.run }
+ @srv.wait_till_running
+ req = EchoMsg.new
+ stub = FailingStub.new(@host, **client_opts)
+ blk = proc { stub.an_rpc(req) }
+
+ # confirm it raise the expected error
+ expect(&blk).to raise_error GRPC::BadStatus
+
+ # call again and confirm exception has the expected fields
+ begin
+ blk.call
+ rescue GRPC::BadStatus => e
+ expect(e.code).to eq(service.code)
+ expect(e.details).to eq(service.details)
+ expect(e.metadata).to eq(service.md)
+ end
+ @srv.stop
+ t.join
+ end
+ end
end
end
--
GitLab