spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.6k stars 38.13k forks source link

`DefaultWebClientBuilder` should support dynamic loading of default `ClientHttpConnector` #28379

Closed bondolo closed 10 months ago

bondolo commented 2 years ago

Affects: 5.3+

Applications creating WebClient instances have the option of either explicitly specifying the ClientHttpConnector to be used for that instance via builder.clientConnector(...) or using a default ClientHttpConnector provided by the builder implementation. The current DefaultWebClientBuilder implementation currently manually chooses the default ClientHttpConnector provider to be used for WebClient instances. The current implementation is hardcoded to recognize specific implementation classes in a preferred order and chooses the first provider which is present on the class-path as the default connection provider.

We have built our own ClientHttpConnector implementation and would like to use it as the default connector in our applications. A current source of errors for our users is forgetting to initialize the WebClient.Builder.clientConnector(new OurClientHttpConenctor()). The need to modify existing application source code in order to user our client connector is also an inconvenience.

DefaultWebClientBuilder should be improved to use a SpringFactoriesLoader, JDK ServiceLoader or other dynamic mechanism for loading/defining the ClientHttpConnector factories and choosing the default factory implementation for an application.

bondolo commented 2 years ago

ping!

bclozel commented 2 years ago

Sorry for the late response.

I'm not sure this would be the preferred way of implementing this use case. For example in Spring Boot, a WebClient.Builder is auto-configured to not only select the right ClientHttpConnector, but also configure client codecs (applying Jackson preferences) and developer-provided customizers. This allows developers to inject this builder anywhere and build an ad-hoc client for the task at hand:

@Component
public class WeatherService {

  private final WebClient client;

  public WeatherService(WebClient.Builder builder) {
    this.client = builder.baseUrl("https://example.org/weather/api/").build();
  }
}

Also, in the case of Netty and Jetty, the client is also auto-configured to reuse the server HTTP resources for better efficiency - such resources cannot always be looked up statically, thus requiring a later phase in the application context lifecycle.

Does this mirror desired features for your OurClientHttpConnector or did you achieve this in some other way?

bondolo commented 2 years ago

In current usage we provide a scope prototype bean that configures and builds a WebClient for injection. The bean is injected with the WebClient.Builder as in your example but sets the ClientHttpConnector to our connector and builds the web client.

WebClient.Builder is auto-configured to not only select the right ClientHttpConnector,

For some definition of "right"… what we are looking for is a mechanism to extend/configure the choice of ClientHttpConnector, everything else is fine and desirable. We even considered packaging our connector an "imposter" that would be recognized as one of the currently supported sources of ClientHttpConnector, but decided that would be unsavory.

This logic in DefaultWebClientBuilder doesn't allow for changing the default even though there is a public builder setter for ClientHttpConnector which is strangely inflexible. The ordering of the choices doesn't seem to reflect a real preference as it can probably be presumed that only one or two of the four sources will typically be on the class path.

private ClientHttpConnector initConnector() {
        if (reactorClientPresent) {
            return new ReactorClientHttpConnector();
        }
        else if (jettyClientPresent) {
            return new JettyClientHttpConnector();
        }
        else if (httpComponentsClientPresent) {
            return new HttpComponentsClientHttpConnector();
        }
        else {
            return new JdkClientHttpConnector();
        }
    }

Even just moving this method into a bean annotated with @ConditionalOnMissingBean so that it can be replaced/overridden would be sufficient.

bclozel commented 10 months ago

Sorry for the delay.

I see how such a solution would be useful but unfortunately this comes with its own set of limitations and maintenance burden. For example, this would mean additional reflection cost, infrastructure to handle this reflection in native apps, classpath ordering issues if several jars define their own, etc. In the end, I think that the auto-configuration mechanism with an ordered WebClientCustomizer in Spring Boot would achieve most of it without disrupting existing apps.

Also, having this kind of service loader mechanism for discovery would be a first in the web space and I don't think we should go there without solid reasons.

I'm declining this issue as a result. Thanks!

bondolo commented 9 months ago

It is disappointing to have had to wait for this answer for more than 18 months. This does seem like a reasonable extension point and using the @ConditionalOnMissingBean approach would have meant that for the core there would be no additional testing; compatibility of the returned connector would be the app's problem. Luckily, I have moved on and this no longer matters to me personally…

bondolo commented 9 months ago

I will say though that following this issue did result in my following the outcome of other people's proposal issues and my impression is to be underwhelmed by the engagement and responsiveness of the Spring Boot team.

I would not encourage someone and would be unlikely myself to bother making a similar proposal in the future. Which is disappointing. Contrast this case to https://github.com/junit-team/junit5/issues/3018, which, though the outcome is the same, shows a lot more engagement. Guess which one I feel better about?

bclozel commented 9 months ago

This does seem like a reasonable extension point and using the @ConditionalOnMissingBean approach would have meant that for the core there would be no additional testing; compatibility of the returned connector would be the app's problem.

We also need to take into consideration the wider ecosystem. Between the factory in Spring Framework, auto-configuration and customizers in Spring Boot, third party libraries, we don't want to increase complexity here (it's a common Spring criticism).

I will say though that following this issue did result in my following the outcome of other people's proposal issues and my impression is to be underwhelmed by the engagement and responsiveness of the Spring Boot team.

To be precise, this is the Spring Framework issue tracker - not that it makes a real difference for this case, but I wouldn't blame the Spring Boot team for this.

I would not encourage someone and would be unlikely myself to bother making a similar proposal in the future. Which is disappointing.

We are very much aware of this problem and we've been working over the last few months on improving our issue triage process. We've made significant progress on our backlog and responsiveness metrics. We are now closing issues more proactively instead of waiting for community demand for long periods of time. We can always reopen issues later, but sending the right message to contributors is much better.

Thanks!