Throw HTTP/400 instead of HTTP/500 for invalid body-dto's (Closes #301)

[bamkrs]

Instead of throwing an HTTP/500 (internal server error) Oat++ can now detect invalid body-dto's and throws an HTTP/400 (bad request) if the body-dto isn't parsable or the wrong body-dto.

There is however, a bug in the tests: After running the 400-Test (test/oatpp/web/FullTest.cpp#305) the following echo-test fails. Does the connection get closed because of a "failed" request?

[lganzzzo]

Hey @bamkrs ,

Just wanted to say that I've seen the PR, but had no chance to look deeply into the test errors.
At first glance, it looks like a connection issue due to the fact that oatpp immediately closes the connection on error before the client could read the response (It seems to me we've discussed this issue some time ago)

Your changes look good.
If it's really the connection issue, we'll have to go deeper to resolve it:)

Please feel free to continue your work in case of any new ideas.
I plan to dive into this issue once I prepare the 1.2.0 must-have list of features.

Best Regards,
Leonid

[bamkrs]

Hey @bamkrs ,

Just wanted to say that I've seen the PR, but had no chance to look deeply into the test errors.
At first glance, it looks like a connection issue due to the fact that oatpp immediately closes the connection on error before the client could read the response (It seems to me we've discussed this issue some time ago)

Yeah I know that we stumbled upon this or a similar problem a while ago. It somehow feels familiar. I think we accepted this behaviour as the desired one (closing internal connection on error) and just removed the test that throws the error that should be tested.

However, maybe the client should automatically reconnect for the next call? I don't know what exactly goes on since its not the next call that fails but the second next or even the third.

[lganzzzo]

Yeah I know that we stumbled upon this or a similar problem a while ago. It somehow feels familiar. I think we accepted this behaviour as the desired one (closing internal connection on error) and just removed the test that throws the error that should be tested.

Despite the fact that we've accepted it last time, I think it's a bug and we should fix it.
Also, I think It must be for "virtual connections" only.

However, maybe the client should automatically reconnect for the next call? I don't know what exactly goes on since its not the next call that fails but the second next or even the third.

It looks weird.
We have a retry policy that client may set for automatic retries. However, I think it should work properly on a single connection when fixed.

[bamkrs]

Yeah I know that we stumbled upon this or a similar problem a while ago. It somehow feels familiar. I think we accepted this behaviour as the desired one (closing internal connection on error) and just removed the test that throws the error that should be tested.

Despite the fact that we've accepted it last time, I think it's a bug and we should fix it.
Also, I think It must be for "virtual connections" only.

So I finally found the root issue. It affects virtual and real connections.
If some code calls the error handling like I do in the macro catching the invalid body-dto

oatpp/src/oatpp/codegen/api_controller/base_define.hpp

Line 229 in c09798a

return ApiController::handleError(Status::CODE_400, buffer.toString()); \

and no error handler is set, a generic HttpError is thrown:

oatpp/src/oatpp/web/server/api/ApiController.cpp

Line 67 in 25e465b

throw oatpp::web::protocol::http::HttpError(status, message);

This exception, however, is not caught in the handler method because we do not have an error-handler set for this ApiController:

oatpp/src/oatpp/web/server/api/ApiController.hpp

Line 286 in 25e465b

if(m_controller->m_errorHandler) {

Thus the exception is caught in HttpProcessor::processNextRequest which returns false when it catches an error.
https://github.com/oatpp/oatpp/blob/master/src/oatpp/web/server/HttpProcessor.cpp#L126

Finally, because HttpProcessor::processNextRequest returns false, the whole processing-task is stopped (quietly)

oatpp/src/oatpp/web/server/HttpProcessor.cpp

Line 196 in 25e465b

wantContinue = HttpProcessor::processNextRequest(resources);

---

So an easy fix would be to change

oatpp/src/oatpp/web/server/HttpProcessor.cpp

Line 130 in 25e465b

return false;

to true since this is only an "harmless" HttpError which is quite normal and HttpErrors are yielded all the time.

However, a more elegant approach would be to catch these "good" errors higher up and just propagate them to the client and keep everything alive.

[lganzzzo]

Yes, you are correct about the root issue.
In case of an error, oatpp drops the connection immediately after writing the response to the socket.
In case of the "real" connection (since the socket still stays alive for some time after the close call) client is able to read the error message. But in the case of the "virtual" connection, the connection gets dropped right away and the client isn't able to get anything from it.

So an easy fix would be to change ... to true since this is only an "harmless" HttpError which is quite normal and HttpErrors are yielded all the time.

Not really - the challenge is the request body.
oatpp doesn't always read requests' body automatically. Instead, it gives full control to the endpoint - when and how to read/stream the requests' body.
If an exception occurs and the endpoint exits (with the body being read at some random position) and we keep the connection alive, then we have two scenarios:

• we read from the connection some garbage left from the unfinished body read, and throw 400 - invalid request.
• the garbage in the connection appears to be a text identical to an HTTP request, which was not intended to be executed.

And the solution here would be whether to always drain the body, or to always drop connection on exceptions (second is what we currently do).

---

So the correct solution here would be:

1. refactor that monstrous method HttpProcessor::processNextRequest
2. track somehow whether the body was drained or not, and drain it before reading the next request.
3. sleep for some time if the connection was dropped by the server-side. linger is not an option since it'll work for "real" connections only.

The good news is that it's not so scary as it sounds.
So yes, that's why this PR is still here. I want to refactor that stuff first. Ideally for 1.2.5

[bamkrs]

Hey @lganzzzo I got a few messages, that you are working on v1.3.0
While you found a workaround for the problem in the tests, whats your opinion on the PR? Should we merge it to v1.3.0 or should we keep it open until the root problem is solved?

[lganzzzo]

Hey @bamkrs ,
Let's keep it open for now
Will see how it goes with 1.3.0