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

Consider providing a property to configure or select the preferred ClientHttpConnector #34962

Closed
ciscoo opened this issue Apr 12, 2023 · 2 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@ciscoo
Copy link

ciscoo commented Apr 12, 2023

Currently, ClientHttpConnectorConfiguration configures a ClientHttpConnector based on what is available on the classpath. This works fine for the main application code, but can become an issue if dependencies bring in dependencies that activate the various client connector configuration.

This is the case when using WireMock. At my company, we use WireMock to mock out underlying backend/downstream calls for our test suites.

When migrating to HTTP Interface clients (from Spring Cloud OpenFeign), I noticed that in the main application code, JdkClient was being configured as I would expect.

But in tests, when WireMock is on the test classpath, HttpClient5 was being configured due to WireMock having a hard dependency on Apache HTTP related components. Attempting to exclude them leads to other classpath issues

I am suspecting that due to the ordering of the configuration where ClientHttpConnectorConfiguration.HttpClient5 is defined before ClientHttpConnectorConfiguration.JdkClient, HttpClient5 gets configured and as a result leads to the following classpath issue during test execution:

java.lang.NoClassDefFoundError: org/apache/hc/core5/reactive/ReactiveResponseConsumer
	at org.springframework.http.client.reactive.HttpComponentsClientHttpConnector.lambda$execute$2(HttpComponentsClientHttpConnector.java:125) ~[spring-web-6.0.7.jar:6.0.7]
	at reactor.core.publisher.MonoCreate.subscribe(MonoCreate.java:58) ~[reactor-core-3.5.4.jar:3.5.4]
	at reactor.core.publisher.MonoDefer.subscribe(MonoDefer.java:52) ~[reactor-core-3.5.4.jar:3.5.4]
...

Caused by: java.lang.ClassNotFoundException: org.apache.hc.core5.reactive.ReactiveResponseConsumer
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) ~[na:na]
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) ~[na:na]
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521) ~[na:na]
	... 67 common frames omitted

Including the following dependency fixes the issue:

testImplementation("org.apache.httpcomponents.core5:httpcore5-reactive") {
    because("Required by WireMock otherwise causes classpath issues")
}

It would be nice if Spring Boot could offer a property to specify the preferred ClientHttpConnector implementation in addition to what is available on the classpath.

The alternatives without the property are:

  1. Attempt to exclude transitive dependencies so that the correct ClientHttpConnector is selected.
  2. Define an explicit bean in the main application code for the preferred implementation (below)
@Bean
public JdkClientHttpConnector jdkConnector() {
    return new JdkClientHttpConnector();
}

Here is a sample project demonstrating the failing test with WireMock. Either define a JdkClientHttpConnector bean in the main code or add the above httpcore5-reactive dependency.

I don't think adding a property would add any meaningful value since the same can be accomplished by declaring an explicit bean, but wanted to hear your thoughts on the proposal.

@wilkinsona
Copy link
Member

Thanks for the report, @ciscoo. Your analysis is correct. In order of preference, the connectors that will be used are the following:

  1. Reactor Netty
  2. Jetty Client
  3. Apache Http Client 5
  4. JDK Client

Defining your own bean is the currently recommended way of overriding this behavior.

java.lang.NoClassDefFoundError: org/apache/hc/core5/reactive/ReactiveResponseConsumer
	at org.springframework.http.client.reactive.HttpComponentsClientHttpConnector.lambda$execute$2(HttpComponentsClientHttpConnector.java:125) ~[spring-web-6.0.7.jar:6.0.7]
	at reactor.core.publisher.MonoCreate.subscribe(MonoCreate.java:58) ~[reactor-core-3.5.4.jar:3.5.4]
	at reactor.core.publisher.MonoDefer.subscribe(MonoDefer.java:52) ~[reactor-core-3.5.4.jar:3.5.4]

This looks like a bug to me. We should not be trying to use the Apache Client unless all of the required classes are available. I have opened #34964.

With regards to the benefits of adding a property, we have similar behavior with RestTemplate. By default, the auto-configured RestTemplateBuilder will detect the client request factory to use based on the classpath. There, the order of preference is the following:

  1. Apache Http Client 5
  2. Ok HTTP
  3. Simple (JDK URLConnection)

Similarly, you can override this behavior by providing your own RestTemplateBuilder bean, configured with your preferred request factory.

I mention the RestTemplate side of things as, if we were to offer a property for ClientHttpConnector, I think we'd want to offer one for the ClientHttpRequestFactory as well.

I'm a little unsure as to how much value a property would offer over defining your own bean. The advantage of the bean-based approach is that it's type safe – you cannot configure a connector or request factory that isn't on the classpath – but a property would make it easy to configure things incorrectly. I'll flag this for discussion with the team so that we can decide what to do.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Apr 13, 2023
@philwebb
Copy link
Member

We discussed this today as a team and decided that we'd rather not offer a property to configure this. Defining a custom bean remains our preferred approach.

@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Jun 14, 2023
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants