Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle routing engine http statuses #1083

Open
FFKL opened this issue Apr 8, 2024 · 7 comments
Open

Handle routing engine http statuses #1083

FFKL opened this issue Apr 8, 2024 · 7 comments
Labels
bug good first issue
Milestone
v1.15.0

Comments

@FFKL
Copy link

FFKL commented Apr 8, 2024

Hello! I migrated vroom from v1.13.0 to v1.14.0 and got a Missing durations error. Then I discovered that the problem is connected to the requirement of adding the postfix ors/v2 to the host setting. But the main problem is that vroom doesn't handle any HTTP status codes, so it's hard to debug such errors. Perhaps a good option would be to throw a RoutingException?

The status just drops

vroom/src/routing/http_wrapper.cpp

Lines 123 to 126 in 1fd711b

// Removing headers.
auto start = response.find('{');
if (start == std::string::npos) {
throw RoutingException("Invalid routing response: " + response);

and I received an error related to the request body

vroom/src/routing/http_wrapper.cpp

Lines 161 to 164 in 1fd711b

if (!json_result.HasMember(_matrix_durations_key.c_str())) {
throw RoutingException("Missing " + _matrix_durations_key + ".");
}

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 9, 2024

Perhaps a good option would be to throw a RoutingException?

Not sure what you mean here since this is already what's happening internally in the two links you shared.

The way this translates to the output is that whenever we catch such an error, the status is set accordingly, see https://github.com/VROOM-Project/vroom/blob/master/docs/API.md#code. So on top of the (short) message you get, you also already get an indication of the origin of the error based on the code value.

@FFKL
Copy link
Author

FFKL commented Apr 9, 2024

@jcoupey Ok, I'll show you the concrete cases.

Case 1 - Confusing message

Routing engine configuration:

ors:
  driving-car:
    host: 'ors-app' # There is only host without the standard base path /ors/v2
    port: '8080'

Request the matrix to http://ors-app:8080/matrix/driving-car

Response body contains html page. Status is 404 - Not found.

<!doctype html><html lang="en"><head><title>HTTP Status 404 – Not Found</title><style type="text/css">body {font-family:Tahoma,Arial,sans-serif;} h1, h2, h3, b {color:white;background-color:#525D76;} h1 {font-size:22px;} h2...

Vroom result:

{ "code": 3, "error": "Missing durations." }

The problem is that the error contains confusing message. I think that the result of the request to ors provider is closer to connection error Failed to connect to host:port then to the original message which implies that the only problem is the ors request body has no durations property. But it's not true. The path does not exist. There is no status code or content-type header check. I suspect that the parser considered the css rules to be json. They start with { and end with }.

Case 2 - Segmentation fault

Routing engine configuration:

ors:
  driving-car:
    host: 'ors-app/ors/v3' # There is a typo in the version. It must be v2.
    port: '8080'

Request the matrix to http://ors-app:8080/ors/v3/matrix/driving-car

Response body is in json format. Status is 404 - Not found.

{
  "timestamp": "...",
  "status": 404,
  "error": "Not Found",
  "path": "/ors/v3/matrix/driving-car"
}

Vroom process crashed with Segmentation fault (core dumped).

Vroom API response:

{ "code": 1, "error": "Internal error" }

I don't think the process should crash because of the wrong path. It would be good to wrap the routing engine error into something more meaningful.

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 10, 2024

Thanks for elaborating on this. I indeed overlooked the initial problem, which is that we should have an additional check to catch the error earlier. I see two ways of doing this:

  1. take the HTTP status into account somehow in HttpWrapper::ssl_send_then_receive;
  2. check for valid json for the json_string returned value.

The first option would require to get the status from asio, probably possible but I never really looked at that. The second option could be easy to setup: currently we only check for json parsing errors in debug mode, see

vroom/src/routing/http_wrapper.cpp

Lines 144 to 148 in 1fd711b

#ifdef NDEBUG
json_result.Parse(json_content.c_str());
#else
assert(!json_result.Parse(json_content.c_str()).HasParseError());
#endif
. This could be turned into a regular check, throwing a RoutingException with a somewhat more explicit message.

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 10, 2024

On the second case: I don't have an ORS setup to reproduce right now but I'm a bit surprised that you don't get a Missing durations proper error. That is if the response is actually valid json (without random html). Printing out the content of the response variable before HttpWrapper::ssl_send_then_receive returns would probably be informative here.

@FFKL
Copy link
Author

FFKL commented Apr 10, 2024

The first option would require to get the status from asio, probably possible but I never really looked at that.

As I understand asio knows nothing about http so we need to parse headers and status code. Anyway I'll try to fix the bug.

@FFKL
Copy link
Author

FFKL commented Apr 10, 2024

On the second case: I don't have an ORS setup to reproduce right now but I'm a bit surprised that you don't get a Missing durations proper error. That is if the response is actually valid json (without random html). Printing out the content of the response variable before HttpWrapper::ssl_send_then_receive returns would probably be informative here.

Not Found ORS error violates their service routing error convention. This is a web framework error and not a matrix service error. The error is a string but not an object so the process crashes.

{
  "timestamp": "...",
  "status": 404,
  "error": "Not Found",
  "path": "/ors/v3/matrix/driving-car"
}

vroom/src/routing/ors_wrapper.cpp

Lines 64 to 71 in 176f57a

void OrsWrapper::check_response(const rapidjson::Document& json_result,
const std::vector<Location>&,
const std::string&) const {
if (json_result.HasMember("error")) {
throw RoutingException(
std::string(json_result["error"]["message"].GetString()));
}
}

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 11, 2024

The error is a string but not an object so the process crashes.

So we should check for the "message" key prior to accessing it, and throw with a different message if it is missing.

@FFKL FFKL mentioned this issue Apr 13, 2024
Open
@jcoupey jcoupey added this to the v1.15.0 milestone Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue
Projects
None yet
Milestone
v1.15.0
Development

No branches or pull requests
2 participants
@jcoupey @FFKL