Fix Empty Responses when IOExceptions Are Thrown in Toadlets#1120
Fix Empty Responses when IOExceptions Are Thrown in Toadlets#1120Bombe wants to merge 17 commits into
Conversation
Unfortunately, it currently looks like we won’t be able to update to a newer version than 3, because Mockito starts using WeakReference as a type for some internal representation or other, so we can’t mock objects anymore that use it.
Bombe
changed the title
The reason that I/O exceptions were swallowed and caused the node to simply not send a reply at all is that IOExceptions in the handle() method were ignored. While the actual reasoning for that is unknown and lost to time, I can imagine that it was assumed that any IOExceptions that occur would do so either while parsing the input stream or while writing to the output stream, thus making it possibly pointless to try and generate a response. This missed the fact that toadlets are allowed to throw IOExceptions in their handleMethod* methods as well, and as soon as that happens, the node would simply drop the connection. Now IOExceptions are handled the same way all (well, most) other exceptions are handled: by returning an HTTP status code of 500, and a nice, text-only page that says “Internal error” and prints the stack trace.
Bombe
force-pushed
the
fix/ioexceptions-in-toadlets
branch
from
c8adc88 to
6539f48
Compare
|
Reviewed 6539f48 in isolation; this looks very much welcome. |
|
Almost works with IO Exceptions. The error is logged but the error response cannot be sent because the socket is already closed. If you log the Exception in the Exception handler where the HTTP response should be sent you will get: (freenet.clients.http.ToadletContextImpl, HTTP socket handler@859137091(3), ERROR): Caught unhandled error in errorhandling: java.net.SocketException: Socket is closed handling socket |
|
Indeed, we are actually closing the socket (by way of closing the socket’s |
This is both clearer and (together with un-finaling PageMaker) allows us to get rid of the Mockito’s inline mock-maker which is problematic with later version of the JDK.
The real problem here is the try-with-resources; both input streams created in the try-with-resources block are FilterInputStream, which close their original streams when they are closed. For an InputStream obtained from a Socket, this means to close the socket, and that breaks both tests and our error handling.
The tests now use real sockets, making for realistic conditions during testing. With mocks, many things would go unnoticed, such as reads or writes after a socket has been closed. The `shutdownOutput` is required to make the tests work; the tests depend on being able to read the input stream before the socket is closed. When the socket is closed as part of closing the input streams, the socket on the test side will get its connection reset, preventing it from reading the rest of the stream.
The socket will be closed outside of the handler.
3 participants
(This PR is built upon PR-1121, because it needs all the test scaffolding from that PR. Merge that one first, this only adds the last commit.)
In
ToadletContextImpl.handle(), in the outer-mosttry-catchblock,IOExceptions are completely ignored and do not generate a response; it was probably assumed thatIOExceptions will always mean that either the input stream or the output stream of the request itself had some kind of issues, in which case generating a response would be mostly useless; however,Toadlets are also allowed to throwIOExceptions from theirhandleMethod*methods. These exceptions now generate an HTTP status 500 response and include a stack trace in the response body.