Skip to content

Find a better way to detect when to start embedded servers #12973

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

Closed
philwebb opened this issue Apr 25, 2018 · 18 comments
Closed

Find a better way to detect when to start embedded servers #12973

philwebb opened this issue Apr 25, 2018 · 18 comments
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@philwebb
Copy link
Member

See https://jira.spring.io/browse/SPR-16760 and rabbitmq/hop#122 for some of the issues we currently have detecting when to start a server for Spring WebFlux.

We need to consider the spring-webflux jar to be more like spring-web and not spring-webmvc. The webflux jar alone is not a strong enough signal that an embedded server should be started.

@philwebb
Copy link
Member Author

From @rstoyanchev on SPR-16760

What if the model was reversed? Make no assumptions from the presence of spring-webflux and instead look for a more explicit indication of the intent to start a WebFlux server. A webflux starter is just such an indication and hence my question. Is there no feasible way to detect what starters were declared, or is the objection purely about making such a check in the first place? Arguably in this case, it would be the very intent to differentiate between the declaration of a starter and the mere presence of a dependency.

In my opinion the starter POMs should not be a direct indication of intent. I think it's important that starters only declare dependencies, and those dependencies are indirectly responsible for auto-configuration. The problem we have with WebFlux is there is no clear indication of intent:

  • spring-webflux = Client and Server components
  • reactor-netty = Client and Server components
  • netty = Client and Server components

SPR-16760 is about splitting spring-webflux but it seem unlikely that we could do that at this point. We could potentially split reactor-netty, but that seems extreme. We've got no chance of splitting netty.

One option we could consider is moving org.springframework.boot.web.embedded to a new jar. This would allow us to have a single dependency that is a strong indication that an embedded server should be started.

@philwebb philwebb added type: task A general task for: team-attention An issue we'd like other members of the team to review labels Apr 26, 2018
@spencergibb
Copy link
Member

Since with tomcat and jetty you have a clear conditional to use.

@philwebb
Copy link
Member Author

philwebb commented May 2, 2018

In Spring Framework 5.1 you'll be able to use WebClient without Netty which may solve one of the main use cases. I'm tempted to say we should do nothing until that's in place and we see if people have any issues after that change.

@philwebb philwebb added status: on-hold We can't start working on this issue yet and removed for: team-attention An issue we'd like other members of the team to review labels May 2, 2018
@philwebb philwebb added this to the Icebox milestone May 2, 2018
@sdeleuze
Copy link
Contributor

sdeleuze commented May 2, 2018

I am not sure to understand how that will help solving this. Spring Framework 5.1 will indeed allow to use Jetty as an alternative to Netty, but Netty should IMO remains the default client engine for various reasons: shared infra with the default server, more mature, more efficient memory allocation since Jetty currently only allows ByteBuffer copy not efficient wrapping. Any thoughts?

@bclozel
Copy link
Member

bclozel commented May 2, 2018

@sdeleuze then I'm not sure what do you have in mind - how could we solve this?

@sdeleuze
Copy link
Contributor

sdeleuze commented May 2, 2018

Sorry in advance if that's a stupid proposal, but what about starting WebFlux server only when we find concrete indication of server-side code in user project. For the functional API, we could rely on something like @ConditionalOnBean(type = "org.springframework.web.reactive.function.server.RouterFunction"), and for the annotation-based model, maybe on something like @ConditionalOnBean(annotation = Controller.class) if meta-annotations are supported?

@philwebb
Copy link
Member Author

philwebb commented May 2, 2018

Unfortunately I don't think that's an option since we need to make the decision about what ApplicationContext to create before the context is populated with bean definitions.

@rstoyanchev
Copy link
Contributor

What about the original suggestion at the top to move org.springframework.boot.web.embedded to a new jar?

@philwebb
Copy link
Member Author

philwebb commented May 2, 2018

That's certainly still an option. I guess we were hoping that using WebClient without Netty was going to become the recommended approach for client only applications.

@philwebb philwebb removed the status: on-hold We can't start working on this issue yet label May 2, 2018
@philwebb philwebb modified the milestones: Icebox, Backlog May 2, 2018
@philwebb
Copy link
Member Author

philwebb commented May 2, 2018

I'll move this back to backlog and think about it some more.

@bclozel
Copy link
Member

bclozel commented May 2, 2018

This would suggest doing one of those:

  1. People would need to add to their build both spring-webflux and reactor-netty dependencies manually if they only want the WebClient. Adding spring-boot-starter-webflux brings that additional org.springframework.boot.web.embedded dependency, turning on the server auto-configuration
  2. Or we need to have two different starters: spring-boot-starter-webflux and spring-boot-starter-webclient

Right now I'm not convinced by either of those.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented May 2, 2018

It's really the case of libraries. Here are two concrete examples that already exist. Spring Integration deps and docs, and Spring Vault deps and docs. Note that there is a ton of spring-projects that use the RestTemplate so there is a strong upside for more such examples over time.

@dsyer
Copy link
Member

dsyer commented May 2, 2018

I'm in favour of option 2. in Brian's list. But to clarify, that also requires splitting out the web.embedded features into a new jar? Or are we comfortable with Rossen's suggestion that the autoconfig detection could be conditional on the presence merely of the starter?

@philwebb
Copy link
Member Author

philwebb commented May 2, 2018

I'm not very keen to use the starter alone as an indication. I'd rather we keep starters purely as dependencies.

@snicoll
Copy link
Member

snicoll commented May 3, 2018

Starters should not be used for anything else than providing dependencies and I think it would be a mistake changing that. As for the client starter, we had that request in the past (for MVC) and declined it independently of this issue.

IMO, the bottom line of this issue is that netty isn't modularized (and so isn't reactor-netty). If things were different, then a scenario à la RestTemplate would be quite consistent. Actually, when WebClient will support other http clients, it will be very consistent with Spring MVC (adding spring-web + http client jar vs. adding spring-webflux and whatever library provides support for Jetty for instance).

I am concerned about the links Rossen has shared. Given that the status quo is to bring a complete server infrastructure with a full server implementation, I wonder if that choice shouldn't be reconsidered.

@sdeleuze
Copy link
Contributor

sdeleuze commented May 3, 2018

Unfortunately I don't think that's an option since we need to make the decision about what ApplicationContext to create before the context is populated with bean definitions.

Point taken, but couldn't we use the same logic programmatically, for example in ReactiveWebServerApplicationContext#onRefresh, to start conditionally the webserver only if there is a RouterFunction bean or at least one bean annotated with @Controller?

@philwebb philwebb added status: on-hold We can't start working on this issue yet type: enhancement A general enhancement and removed type: task A general task labels Jun 1, 2018
@philwebb philwebb self-assigned this Aug 31, 2018
@philwebb philwebb removed this from the Backlog milestone Aug 31, 2018
@gfinger
Copy link

gfinger commented Feb 15, 2023

This would suggest doing one of those:

  1. People would need to add to their build both spring-webflux and reactor-netty dependencies manually if they only want the WebClient. Adding spring-boot-starter-webflux brings that additional org.springframework.boot.web.embedded dependency, turning on the server auto-configuration
  2. Or we need to have two different starters: spring-boot-starter-webflux and spring-boot-starter-webclient

Right now I'm not convinced by either of those.

I would be happy with either option, as I am struggling to keep my client library for ArcadeDB clean of any dependency to a web-server. An application using my library is free to choose to start a server or not, but my lib should not force the application to handle a dependency to a server if not needed.

The above mentioned issue #34198 was opened by me, because I think a proper autoconfiguration should configure and start a server only if it is in the classpath.

@philwebb philwebb removed their assignment Jun 14, 2024
@wilkinsona wilkinsona added status: superseded An issue that has been superseded by another and removed status: on-hold We can't start working on this issue yet labels Apr 25, 2025
@wilkinsona wilkinsona removed this from the 3.x milestone Apr 25, 2025
@wilkinsona
Copy link
Member

This will be addressed as part of the restructuring work for 4.0. The auto-configuration for WebClient and for the embedded servers will be in separate modules making the classpath provide a much stronger signal about the user's intent.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

9 participants