Fixed issue where a 200 response is returned when server throws exception #272
Conversation
…m() to return an error rather than a 200.
|
Thanks for the PR, @mikaelq. This does seem to be a bug. However, we should not just marshal all exceptions and return them. The correct way to implement this is to rely on the update: I've updated the PR description to link to the new issue I created. |
| @@ -215,6 +215,9 @@ public void proxyStream(InputStream input, OutputStream output, Context context) | |||
| } catch (JsonMappingException e) { | |||
| log.error("Error while mapping object to RequestType class", e); | |||
| getObjectMapper().writeValue(output, exceptionHandler.handle(e)); | |||
| } catch (Exception e) { | |||
| log.error("Error while proxying request.", e); | |||
| getObjectMapper().writeValue(output, exceptionHandler.handle(e)); | |||
There was a problem hiding this comment.
See comment on the PR. We should rely on the ExceptionHandler object instead like we do in the proxy() method.
…ervletResponseWriter.failure() to throw if an error occured rather than return ultimately returning a 200
|
@sapessi thanks for the quick reply. Oops, I didn't see The issue is the |
| throw new InternalServerErrorException(e); | ||
| } | ||
| log.error("failure", throwable); | ||
| throw new InternalServerErrorException("Jersey failed to process request", throwable); |
There was a problem hiding this comment.
Wondering what will happen to the latch in this case. The behavior seems correct as far as the outcome (what we return to the client). I'll add a TODO list item to verify that we don't leak anything in this case. I'll take care of it before the next release
There was a problem hiding this comment.
Yeah, I tested that latch.await() is not called during unit tests but this might not be the case every time. Looking closer at the code it could cause a leak. I took a look at Jersey's ApplicationHandler.java they use a future and call .completeExceptionally(). Maybe the code should be refactored to use futures instead of a latch. Also I wonder how framework failures are handled by the other consumers of LambdaContainerHandler.java.
There was a problem hiding this comment.
I've created issue #276 to track this for the 2.0 release of the framework.
|
I'll merge this manually in the |
|
I've merged this manually in the core branch. Thanks for the fix @mikaelq! |
Added handling of all exceptions in LambdaContainerHandler.proxyStream() to return an error rather than a 200.
Issue #, if available: #273
Description of changes:
If an exception is thrown that is not explicitly handled by the LambdaContainerHandler.proxyStream() method an empty 200 will be returned. I've only seen this behavior using the Jersey handler but I believe this will also occur in the other handlers. The Jersey handler returns empty 200s if HK2 is unable to resolve dependencies. If the service framework is unable to start I'd expect a 500 to be returned.