-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Requests with ReactiveElasticsearchRepository methods doesn't fail if it can't connect with Elasticsearch #1712
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
Comments
I can reproduce this with you demo like you described. I still have to investigate why the dropped connection is not reported.
Is it an option to upgrade your application to the actual Spring Boot version? I do not know currently if it is a change in Spring Data Elasticsearch or in Spring webflux that now shows the correct behaviour, I'll have to investigate that. |
Thanks @sothawo. Yes, it seems with Spring Boot 2.4.3 an error is thrown. About the possibility of upgrading, I'm not sure if it's a possibility for us, I'm starting to investigate which changes we'd require for doing so, the service in which we found the issue is in production and uses 2.3.9.RELEASE and it seems there're a lot of changes between the different dependencies in sprint 2.4. |
thanks for the feedback; I'll dig into this after work today. |
Thanks @sothawo. |
Debugging reactive code is ... well, not nice. What I found out is, that the problem comes from a function deep in the Spring webflux code, namely This is from version 5.2.13.RELEASE, pulled in by Spring Boot 2.4.3: public Mono<ClientResponse> exchange(ClientRequest clientRequest) {
Assert.notNull(clientRequest, "ClientRequest must not be null");
HttpMethod httpMethod = clientRequest.method();
URI url = clientRequest.url();
String logPrefix = clientRequest.logPrefix();
return this.connector
.connect(httpMethod, url, httpRequest -> clientRequest.writeTo(httpRequest, this.strategies))
.doOnRequest(n -> logRequest(clientRequest))
.doOnCancel(() -> logger.debug(logPrefix + "Cancel signal (to close connection)"))
.map(httpResponse -> {
logResponse(httpResponse, logPrefix);
return new DefaultClientResponse(
httpResponse, this.strategies, logPrefix, httpMethod.name() + " " + url,
() -> createRequest(clientRequest));
});
} There is no error handling here, the closed connection is not handled, and an empty Mono is returned. In version 5.3.0.RC1 this was changed to @Override
public Mono<ClientResponse> exchange(ClientRequest clientRequest) {
Assert.notNull(clientRequest, "ClientRequest must not be null");
HttpMethod httpMethod = clientRequest.method();
URI url = clientRequest.url();
String logPrefix = clientRequest.logPrefix();
return this.connector
.connect(httpMethod, url, httpRequest -> clientRequest.writeTo(httpRequest, this.strategies))
.doOnRequest(n -> logRequest(clientRequest))
.doOnCancel(() -> logger.debug(logPrefix + "Cancel signal (to close connection)"))
.onErrorResume(WebClientUtils::shouldWrapException, t -> wrapException(t, clientRequest))
.map(httpResponse -> {
logResponse(httpResponse, logPrefix);
return new DefaultClientResponse(
httpResponse, this.strategies, logPrefix, httpMethod.name() + " " + url,
() -> createRequest(clientRequest));
});
} Here now the error is wrapped in an exception. Since 5.3.2 this is changed to @Override
public Mono<ClientResponse> exchange(ClientRequest clientRequest) {
Assert.notNull(clientRequest, "ClientRequest must not be null");
HttpMethod httpMethod = clientRequest.method();
URI url = clientRequest.url();
String logPrefix = clientRequest.logPrefix();
return this.connector
.connect(httpMethod, url, httpRequest -> clientRequest.writeTo(httpRequest, this.strategies))
.doOnRequest(n -> logRequest(clientRequest))
.doOnCancel(() -> logger.debug(logPrefix + "Cancel signal (to close connection)"))
.onErrorResume(WebClientUtils.WRAP_EXCEPTION_PREDICATE, t -> wrapException(t, clientRequest))
.map(httpResponse -> {
logResponse(httpResponse, logPrefix);
return new DefaultClientResponse(
httpResponse, this.strategies, logPrefix, httpMethod.name() + " " + url,
() -> createRequest(clientRequest));
});
} So Im afraid there is nothing in Spring Data Elasticsearch I can do to handle this error as I don't get any information about it from the underlying Spring WebFlux framework. |
Thanks @sothawo for looking into this, I really appreciate it. Following your insight I did some more debug trying to understand the problem. This is what I found: It seems the error reaches the method Also setting
after that I can't understand what's happening, it seems the error is lost somehow. Most likely I'm wrong, but it seems as if the error reaches the Spring Data Elasticsearch layer but somehow it's not propagated, what do you think? does this make sense?. |
have to check that as well, especially where this 503 comes from. Normally a 503 comes from the server with an error message, here there is an empty body as well. And this code does not come from the server, as there is no server available anymore. But you're right, at least this error - no matter where it comes from - should come up at the client call. |
I will change the In your case this will then be translated to an In the current version this comes up as a network error anyway. |
@sothawo Awesome, I just tested it with our service and I can confirm you it solves our issue. Thank you very much 😄. |
Spring Boot: 2.3.9.RELEASE
Spring Data Elastic Search: 4.0.7.RELEASE
Problem
When doing a search request with a ReactiveElasticsearchRepository method, as for example
findAllById()
, if there's no connection with Elasticsearch, for example the cluster is stopped just before the request is made, the connection error is silenced and the request finish correctly but without returning values.How to reproduce it:
Result: The request won't fail and an empty list will be returned.
I wasn't able to prepare a test as I couldn't pinpoint exactly where the problem occurs, but I prepared a project with this example TestBug.test(), it's a little clumsy, it requires to stop the execution with a breakpoint, sorry for that, but is the only way I found to reproduce it, I hope it helps.
The text was updated successfully, but these errors were encountered: