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.

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

empty body causes ErrorKind::Internal #112

Open
mdaniel opened this issue Jan 20, 2019 · 2 comments
Open

empty body causes ErrorKind::Internal #112

mdaniel opened this issue Jan 20, 2019 · 2 comments
Labels
bad error handling A tag to encompass all areas where toshi does not handle errors well or correctly good first issue Good for newcomers

Comments

@mdaniel
Copy link
Contributor

mdaniel commented Jan 20, 2019

What happened

Accidentally omitting document content returns 500 Internal Server Error with a body of {"message":"Internal error","uri":"/new_index"}

What was expected

Emitting any kind of helpful message would be helpful. Also, in my experience, when the client receives a 500 response, there is usually something informative on the server-side. But in this case, the server emits the same message that the client receives, which isn't helpful.

This bug is actually just the worst offender of a whole class of bugs where if something doesn't go Toshi's way, it just gives back a raspberry, but I'd say getting a 500 for an empty document is pretty far up the list for me

How to reproduce

Assuming you create an index based on the cargo test schema, then send in an indexing request of the form

$ echo '{}' | curl ... -X PUT -d @- 127.0.0.1:9200/new_index
@hntd187 hntd187 added good first issue Good for newcomers bad error handling A tag to encompass all areas where toshi does not handle errors well or correctly labels Jan 21, 2019
@hntd187
Copy link
Member

hntd187 commented Jan 21, 2019

And so was born the "bad error handling" label, thanks. There are tons of places in Toshi where I either ignored or didn't spend time doing proper error handling for these kinds of issues. It's very good to point these out as it helps us track down where to add better error handlers to the handles. I think this is pretty straightforward the deserialize from serde to a tantivy Schema fails, so one would need to catch that and respond appropriately.

@Chopinsky
Copy link

Chopinsky commented Oct 10, 2019

Not sure if this is related, but when I was trying to reproduce the test case from the issue, the server panic in one of its worker thread (it's actually from the tokio-runtime-worker).

The command I used was:
curl -X PUT http://localhost:8080/new_index -d '{}'

And I only receive an empty reply form server, while the server console printed out the panic info:

Oct 09 22:51:34.450 INFO toshi::router: REQ = ["new_index"]

thread 'tokio-runtime-worker-2' panicked at 'called 'Result::unwrap()' on an 'Err' value: Error("missing field 'document'", line: 1, column: 2)', src\libcore\result.rs:1084:5

I think the panic was thrown from the line below, where a possible error was unwrapped:

handler.create_index(Body::from(schema), "new_index".into()).wait().unwrap();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bad error handling A tag to encompass all areas where toshi does not handle errors well or correctly good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants