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

Make ReactorClientHttpConnector lifecycle-aware #31180

Closed
sdeleuze opened this issue Sep 6, 2023 · 6 comments
Closed

Make ReactorClientHttpConnector lifecycle-aware #31180

sdeleuze opened this issue Sep 6, 2023 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@sdeleuze
Copy link
Contributor

sdeleuze commented Sep 6, 2023

In order to improve the compatibility with JVM Checkpoint Restore (aka CRaC), ReactorClientHttpConnector should implement the Lifecycle interface.

@sdeleuze sdeleuze added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Sep 6, 2023
@sdeleuze sdeleuze added this to the 6.1.0-RC1 milestone Sep 6, 2023
@sdeleuze sdeleuze self-assigned this Sep 6, 2023
@sdeleuze sdeleuze changed the title Make ReactorClientHttpConnector lifecycle-aware Make ReactorClientHttpConnector lifecycle-aware Sep 6, 2023
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 6, 2023
With this commit, ReactorClientHttpConnector now implements
SmartLifecycle which optionally allows recreating the HttpClient
after ReactorResourceFactory has been updated.

Closes spring-projectsgh-31180
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 6, 2023
With this commit, ReactorClientHttpConnector now implements
SmartLifecycle which optionally allows recreating the HttpClient
after ReactorResourceFactory has been updated.

Closes spring-projectsgh-31180
@agorbachenko
Copy link

@sdeleuze Does it make module spring-web dependency on spring-context mandatory instead of not optional? My project uses ReactorClientHttpConnector explicitly and before spring 6.2 spring-context wasn't required, but now it is (java.lang.NoClassDefFoundError: org/springframework/context/SmartLifecycle)

@bclozel
Copy link
Member

bclozel commented Jan 18, 2024

This change makes this class depend on spring-context, but spring-web has been optionally depending on spring-context for years. As of Spring Framework 6.1, you'll need spring-context on the classpath to use this class.

@sdeleuze
Copy link
Contributor Author

I have updated the upgrade notes accordingly.

@filiphr
Copy link
Contributor

filiphr commented Feb 8, 2024

I have a similar problem like @agorbachenko. We were using only the spring-webflux dependency with manually creating our own ReactorClientHttpConnector with HttpClient. With the change here it means that we now need spring-context to keep using this.

I also had a look into this a bit and I don't quite understand how this works for the intended purposes? Let's say I want to use the WebClient.Builder to create my own instance of the WebClient

public MyBean myBean(WebClient.Builder webClientBuilder) {
    // configure the builder for my purpose
    WebClient webClient = webClientBuilder.build();

    return new MyBean(webClient);
}

Will CRaC work in this example? From what I could see in the smoke tests WebClientConfiguration in the smoke tests the auto configured ClientHttpConnector is injected and only this approach will work with CRaC, right?

Does it make more sense to have a FactoryBean for the ResourceFactory that would implement the Lifecycle and maybe have a wrapper for a ClientHttpConnector that would allow for starting / stopping the ReactorClientHttpConnectorFactory? Or maybe have 2 types, one that is for externally managed and another that is the one from 6.0?

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Feb 9, 2024

Will CRaC work in this example?

Yes, and that's a good point, I have refined the smoke test to not customize the ClientHttpConnector anymore. This is possible thanks to Spring Boot configuring the connector at builder level here.

That's IMO one more reason to keep things as they are implemented, and just add the additional spring-context dependency which will just be used for SmartLifecycle and related types.

@filiphr
Copy link
Contributor

filiphr commented Feb 9, 2024

Thanks for the pointer @sdeleuze. I missed that class, seems like IntelliJ isn't finding that usage if I use the auto configure as a dependency only.

We went with adding the spring-context in this scenario, it makes sense to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants