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

Async methods throw exceptions on all 4xx/5xx errors #654

Closed
Aaronaught opened this issue May 9, 2014 · 3 comments
Closed

Async methods throw exceptions on all 4xx/5xx errors #654

Aaronaught opened this issue May 9, 2014 · 3 comments

Comments

@Aaronaught
Copy link

The documentation states that HTTP errors do not throw exceptions, and this is true for the synchronous methods and was true in the much older (0.20) version of NEST.

However, in the current version (1.0.0-beta1), the async methods will always throw an exception if a 4xx/5xx status is encountered.

Simple repro: execute await client.Raw.DeleteAsync(index, type, id) where the combination of index, type, id points to a document that doesn't exist. This will generate a 404 error and subsequently throw a MaxRetryException.

Stepping further into the source, the retry logic is being triggered because SuccessOrKnownError is false on the response, which is because the HttpStatusCode is actually null. I think the exception is actually bubbling up directly from the WebRequest.EndGetResponse and simply not being caught properly by the task continuation logic.

At the moment I can't use the async methods because of this. Even doing a HEAD request (i.e. DocumentExistsAsync) will throw an exception on a 404, which doesn't make a whole lot of sense.

I assume this is a bug and not an intentional design change?

@Mpdreamz Mpdreamz added the Bug label May 13, 2014
@Mpdreamz
Copy link
Member

This is very much a bug in fact both examples you give still return data on a 404 and so .IsValid should be true for both.

DeleteAsync will return a response with a found property on a 404 and DocumentExistsAsync an exists property that should be checked.

@Mpdreamz
Copy link
Member

Ok several things went wrong here:

  • NEST was in charge of saying which endpoints allowed 404's. This is now part of the code generation for Elasticsearch.NET. e.g a 404 on delete should still be considered .Success == true or .IsValid == true no matter if you use NEST or Elasticsearch.NET.
  • Async methods did not properly materialize ElasticsearchServerExceptions on clients configured with ThrowOnElasticsearchServerExceptions() or set them properly on .OriginalException on clients that do not throw.

https://github.com/elasticsearch/elasticsearch-net/blob/master/src/Tests/Nest.Tests.Integration/Reproduce/Reproduce654Tests.cs

Tests all the possible combinations for NEST/Elasticsearch.NET calling endpoints that should be valid or not both with a client that throws and one that does not.

@Mpdreamz
Copy link
Member

Oh yeah huge 👍 💯 on finding this @Aaronaught

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants