Skip to content

Commit 3ee49c9

Browse files
committed
Controller unhandled errors should be passed as BadStatus to interceptors
While using `Gruf::Interceptors::Instrumentation::RequestLogging::Interceptor`, I noticed that in the event of of unhandled errors raised in my controller (errors that are not of type `GRPC::BadStatus` or raised via `fail!`), I was not getting any request logging in my log. In a successful controller call I would get something like: ``` [GRPC::Ok] (list_units) [5.24ms] [GRPC::Ok] (rpc.units.list_units) Parameters: {:is_active=>false, :check_can_delete=>false} ``` If an unhandled error was raised I would get nothing. My assumption is that I should still get logging in that case. Request info is useful to have, including when an unhandled error occurs. Investingating why I wasn't getting any, I observed that in `Gruf::Interceptors::Instrumentation::RequestLogging::Interceptor`, the interceptor yields to the controller via this line: ```ruby result = Gruf::Interceptors::Timer.time(&block) ``` `Gruf::Interceptors::Timer.time` knows to `rescue` and handle errors, but only of type `GRPC::BadStatus`. I realized that unhandled errors would not be caught by it, hence the error logging interceptor would get exited right there and never log. I noticed that the base controller code turns unhandled errors in the `GRPC::BadStatus` errors. But it only does so after all the interceptors have run (or been tripped as above). I thought why not just do this before the interceptors. Seeing how the logging interceptor is designed, maybe it was even meant to be that way. So this is what I propose here. It works well for me so far. API changes: * Now interceptors will get a `GRPC::BadStatus` error in the case of unhandled errors, instead of the original unhandled error. To reach the original error, `GRPC::BadStatus#cause` can be used.
1 parent e8947be commit 3ee49c9

3 files changed

Lines changed: 29 additions & 6 deletions

File tree

CHANGELOG.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,19 @@ Changelog for the gruf gem. This includes internal history before the gem was ma
22

33
### Pending release
44

5+
* [#184] [Breaking] Controller unhandled errors should be passed as BadStatus to interceptors.
6+
* Server interceptors should now expect a `GRPC::BadStatus` error in the case of unhandled errors.
7+
Orignal unhandled error can be reached via `cause`. example:
8+
```ruby
9+
def call
10+
begin
11+
yield
12+
rescue => err
13+
err.cause
14+
end
15+
end
16+
```
17+
518
### 2.20.1
619

720
* [#208] Fix rails `clear_active_connections!` deprecation warning

lib/gruf/controllers/base.rb

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,13 @@ def process_action(method_key, &block)
9292
def call(method_key, &block)
9393
Interceptors::Context.new(@interceptors).intercept! do
9494
process_action(method_key, &block)
95+
rescue GRPC::BadStatus
96+
raise # passthrough, to be caught by Gruf::Interceptors::Timer
97+
rescue GRPC::Core::CallError, StandardError => e # CallError is not a StandardError
98+
set_debug_info(e.message, e.backtrace) if Gruf.backtrace_on_error
99+
error_message = Gruf.use_exception_message ? e.message : Gruf.internal_error_message
100+
fail!(:internal, :unknown, error_message)
95101
end
96-
rescue GRPC::BadStatus
97-
raise # passthrough, to be caught by Gruf::Interceptors::Timer
98-
rescue GRPC::Core::CallError, StandardError => e # CallError is not a StandardError
99-
set_debug_info(e.message, e.backtrace) if Gruf.backtrace_on_error
100-
error_message = Gruf.use_exception_message ? e.message : Gruf.internal_error_message
101-
fail!(:internal, :unknown, error_message)
102102
end
103103
end
104104
end

spec/gruf/controllers/base_spec.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,16 @@
9797
end
9898
end
9999

100+
it 'raises a GRPC::Internal error to interceptors' do
101+
Gruf.interceptors.use(TestServerInterceptor)
102+
expect_any_instance_of(TestServerInterceptor).to receive(:call) do |&block|
103+
error = nil
104+
expect { block.call }.to raise_error(GRPC::Internal) { |err| error = err }
105+
raise error if error
106+
end
107+
expect { subject }.to raise_error(GRPC::Internal)
108+
end
109+
100110
context 'when backtrace_on_error is set to true' do
101111
before do
102112
Gruf.backtrace_on_error = true

0 commit comments

Comments
 (0)