added error handling #98
Conversation
| // } finally { | ||
| // detachContext(contextWithHeaders, previous); // detach headers from gRPC context | ||
| // } | ||
| // } |
There was a problem hiding this comment.
with close() we can get the exact Status and print it in the access logs.
Need to check how to get this with onCancel() because close() is not guaranteed to be called always according to the javadocs.
| @@ -168,10 +200,10 @@ public void onMessage(Req request) { | |||
| public void onCancel() { | |||
| Context previous = attachContext(contextWithHeaders); // attaching headers to gRPC context | |||
| try { | |||
| grpcFilters.forEach(filter -> filter.doHandleException(new Exception())); | |||
There was a problem hiding this comment.
any better way to get more specific exception/grpc status
| } catch (RuntimeException ex) { | ||
| handleException(call, ex); | ||
| grpcFilters.forEach(filter -> filter.doHandleException(ex)); | ||
| grpcFilters.forEach(filter -> filter.doHandleException(new Exception())); |
There was a problem hiding this comment.
check how to set more descriptive exception containing grpc status code here, like DEADLINE_EXCEEDED
| super.onComplete(); | ||
| grpcFilters.forEach(Filter::doEndFilter); | ||
| } catch (Exception e) { | ||
| handleException(call, grpcFilters, e); |
There was a problem hiding this comment.
we shouldn't be calling handleException here. HandleException is for upstream. For Filter thrown error that needs to be returned without giving it back to the same filter.
| grpcFilters.forEach(filter -> filter.doHandleException(new Exception())); | ||
| grpcFilters.forEach(Filter::doEndFilter); | ||
| } catch (Exception e) { | ||
| handleException(call, grpcFilters, e); |
There was a problem hiding this comment.
Same here, we shouldn't be calling handleException here. HandleException is for upstream. For Filter thrown error that needs to be returned without giving it back to the same filter.
|
|
||
| /** | ||
| * Method for performing ending functions for a filter. | ||
| */ | ||
| public void doEndFilter() {} |
There was a problem hiding this comment.
Not in favor of this. We already have an destroy method. Lifecycle methods should be used only for lifecycle and not for logic.
There was a problem hiding this comment.
This is not for ending lifecycle. This is for printing logs once only.
| @@ -91,19 +92,23 @@ public final void doFilter(ServletRequest request, ServletResponse response, | |||
| .stream().collect(Collectors.toMap(String::toLowerCase, httpServletResponse::getHeader)); | |||
| filters.forEach(filter -> filter.doProcessResponseHeaders(responseHeaders)); | |||
| responseHeaders.forEach(responseWrapper::setHeader); | |||
| filters.forEach(filter -> filter.doProcessResponse(responseWrapper)); | |||
There was a problem hiding this comment.
This will cause filter thrown exception to be caught. We don't want that. Filter thrown error should not be caught
| @@ -57,7 +58,7 @@ public class AccessLogGrpcFilter<R extends GeneratedMessageV3, S extends Generat | |||
| private static final Logger logger = Logging.loggerWithName("ACCESS-LOG"); | |||
|
|
|||
| public AccessLogGrpcFilter() { | |||
|
|
|||
| accessLogContextBuilder.requestTime(System.currentTimeMillis()); | |||
There was a problem hiding this comment.
Init accessLogContextBuilder here and set requestTime
| implementation ('ru.vyarus:guice-validator:1.2.0') { | ||
| exclude group: 'com.google.inject', module: 'guice' | ||
| } | ||
| implementation 'com.google.inject:guice:5.1.0' |
No description provided.