helidon-io / helidon

Java libraries for writing microservices
https://helidon.io
Apache License 2.0
3.48k stars 562 forks source link

Default error handler does not return a 5xx on UncheckedIOException #9192

Closed manger closed 2 weeks ago

manger commented 3 weeks ago

Environment Details


Problem Description

When a request handler throws an UncheckedIOException (such as when the handler uses a WebClient to call another service that isn't currently running resulting in a ConnectException wrapped in an UncheckedIOException) I expect the Helidon default error handler to return a 500 HTTP error. Instead the web server return nothing.

The cause is io.helidon.webserver.http.ErrorHandlers.runWithErrorHandling line 80 that catches and re-throws UncheckedIOException (and CloseConnectionException), with a comment saying 'these errors must "bubble up"'.

Maybe if the response is "broken" so returning an HTTP 500 can't work then an exception needs to bubble up. Perhaps the code assumes that these 2 exception categories (UncheckedIOException and CloseConnectionException) are due to the response. But that is not always correct, particularly since the Helidon WebClient throws those and lots of microservices call other microservices.

Steps to reproduce

Expect HTTP/1.1 500 Internal Server Error but don't get anything.

tomas-langer commented 3 weeks ago

There are a few exception that you cannot throw from your code, and UncheckedIOException is one of them (edit: you can of course throw it, but it will close the connection and return nothing, it is also never logged, as otherwise your log would be full of closed connections messages.). We need this exception to "bubble" through, as this is the only way how we identify issues where (for example) the client closed the connection, and we failed to write a response.

You can use checked exception in Helidon routing, so you can throw IOException.

manger commented 3 weeks ago

Treating every UncheckedIOException as if it is about the incoming request/response is a poor assumption. Plenty of other I/O can occur when handling a request. One microservice calling another or calling a database is really common -- and code for that also throws the quite-generic UncheckedIOException. Helidon's WebClient throws UncheckedIOException. Having to wrap all those potential sources with try-catch blocks to re-package UncheckedIOExceptions is painful, will be error-prone, defeats the purpose of the "Unchecked" part, and forces a surprising (and undocumented) rule on every handler (don't throw UncheckedIOException if its not about the response connection).

Perhaps Helidon could use it's own special exception to signal response connection problems to error handling routing?

tomas-langer commented 2 weeks ago

I will update this as you proposed - using a custom exception. Will require some time, as it impacts quite a lot of code. Also I need to try to make this backward compatible...

manger commented 1 week ago

Thanks a lot for the prompt response and good solution.