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

Neo4j reactive health indicator is preferred when Reactor is on the classpath #24838

Closed
mrksph opened this issue Jan 14, 2021 · 6 comments
Closed
Labels
status: duplicate A duplicate of another issue

Comments

@mrksph
Copy link

mrksph commented Jan 14, 2021

Hi,

This issue is related to spring-projects/spring-data-neo4j#2116

I have a project that uses Spring Boot 2.4.1 and Neo4J as a database. I also use Spring Data Neo4j 6.0. Right now we are migrating from Neo4j v3.5 to v4.2.
When I try to connect to the older version I get the following exception:

org.neo4j.driver.exceptions.ClientException: Driver is connected to the database that does not support driver reactive API. In order to use the driver reactive API, please upgrade to neo4j 4.0.0 or later.
	at org.neo4j.driver.internal.cursor.AsyncResultCursorOnlyFactory.rxResult(AsyncResultCursorOnlyFactory.java:80) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1146) ~[na:na]
	at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:506) ~[na:na]
	at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2137) ~[na:na]
	at org.neo4j.driver.internal.util.Futures.lambda$asCompletionStage$0(Futures.java:93) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:577) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:551) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.DefaultPromise.access$200(DefaultPromise.java:35) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.DefaultPromise$1.run(DefaultPromise.java:501) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:164) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:472) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:500) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at org.neo4j.driver.internal.shaded.io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[neo4j-java-driver-4.1.1.jar:4.1.1-eac256ab0fe1a26e16b8c683fb90af7d3e0c471c]
	at java.base/java.lang.Thread.run(Thread.java:832) ~[na:na]

I don't use Reactor in my project but it's in the classpath as a transitive dependency, in this case, coming from Spring Integration Test (yeah, I know it should be scoped as a test dependency but that's not the matter, any other dependency that brings Reactor would produce the same bug).

After researching for a while I found that Spring Actuator, as part of its Health Check process, checks for Reactor in the classpath like this in HealthEndpointConfiguration class:

    @Bean
    @ConditionalOnMissingBean
    HealthContributorRegistry healthContributorRegistry(ApplicationContext applicationContext,
        	HealthEndpointGroups groups) {
        Map<String, HealthContributor> healthContributors = new LinkedHashMap<>(
        		applicationContext.getBeansOfType(HealthContributor.class));

        if (ClassUtils.isPresent("reactor.core.publisher.Flux", applicationContext.getClassLoader())) {
        	healthContributors.putAll(new AdaptedReactiveHealthContributors(applicationContext).get());
        }

        return new AutoConfiguredHealthContributorRegistry(healthContributors, groups.getNames());
    }

This piece of code has a huge impact because it instantiates an AdaptedReactiveHealthContributors that ends up querying the database as a Reactive component when the old version of Neo4j doesn't support reactive API.

Here's the commit that adds the check

ae5ae72

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 14, 2021
@mrksph mrksph changed the title Spring Boot Actuator Autoconfigure wrongly detects Reactor in classpath when it is a transitive dependency Spring Actuator wrongly detects Reactor in classpath as usage when it is a transitive dependency not used in the project. Jan 14, 2021
@snicoll
Copy link
Member

snicoll commented Jan 14, 2021

Thanks for the report but I don't think this analysis is accurate.

I don't use Reactor but it is in the project classpath as a transitive dependency

Unfortunately, it doesn't matter that it is a transitive dependency. Once Reactor is on the classpath, a number of available integrations will be enabled, including Neo4j. In the case of Neo4j, the new Driver implements both the imperative and the reactive variant with the same code so the infrastructure is available as soon as reactor is around. As you've noticed yourself, the reactive driver requires you to target a new Neo4j server version.

This was raised before and there are some ideas we'd like to explore in #22692.

In the meantime you can either exclude reactor from your project or you can opt-in for the imperative variant of the health indicator by registering a bean for it:

@Bean
public Neo4jHealthIndicator neo4jHealthContributor(Driver driver) {
    return new Neo4jHealthIndicator(driver); 
}

Let us know how it goes.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 14, 2021
@snicoll snicoll changed the title Spring Actuator wrongly detects Reactor in classpath as usage when it is a transitive dependency not used in the project. Neo4j reactive health indicator is preferred when Reactor is on the classpath Jan 14, 2021
@mrksph
Copy link
Author

mrksph commented Jan 14, 2021

Thanks for pointing it out, I will check the issue.

Btw, this is not blocking my project, in fact, we are migrating from the older version and as part of the migrating process we needed to check the new code against the old version and that's when I came upon this.

I think that, for now, I will remove Spring Integration Test dependency (not using it) and keep a note on what you said on how to force the Imperative variant of the Health Indicator.

Yeah, looking on the classpath for the Reactor library is not only limited but I think is also fragile. Having to registrate a Bean to specifically opt-in for the imperative infra don't look good to me but I understand it's what we have to work with for now :)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 14, 2021
@snicoll
Copy link
Member

snicoll commented Jan 14, 2021

Alright, thanks for the quick follow-up.

Closing as a duplicate of #22692.

@snicoll snicoll closed this as completed Jan 14, 2021
@snicoll snicoll added status: duplicate A duplicate of another issue and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jan 14, 2021
@mrksph
Copy link
Author

mrksph commented Jan 14, 2021

One last question: is there any way I could help you solve that issue? It looks like an interesting problem but from my understanding of Spring internals, it looks too obscure to me to even know what should I change.

@snicoll
Copy link
Member

snicoll commented Jan 14, 2021

Thanks for asking and offering to help. The issue I've linked to has status: pending-design-work on it, which is an indication we don't know ourselves yet. I suspect we'll surface some way for users to opt-in for imperative or reactive style which would let you control this via properties.

@mrksph
Copy link
Author

mrksph commented Jan 14, 2021

Thanks for the quick reply.

Yeah, that's what I've read in the description and it looks nice.
I'll keep an eye on it to see what are the proposed changes so maybe I can be of any help in the future.

Have a good day! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

3 participants