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

Clarify the need for Spring WebFlux #122

Closed
snicoll opened this issue Apr 13, 2018 · 18 comments
Closed

Clarify the need for Spring WebFlux #122

snicoll opened this issue Apr 13, 2018 · 18 comments
Milestone

Comments

@snicoll
Copy link

snicoll commented Apr 13, 2018

I am a bit confused why this project relies on Spring and, in particular, Spring WebFlux. This doesn't sound right for something that can be used as a client to rabbit.

Due to the current dependency structure, simply adding this jar will signal to Spring Boot that it has to start a web server. One might argue that the sole presence of spring-webflux is too weak but that ship has sailed unfortunately.

What is the rationale for using Spring WebFlux?

@snicoll snicoll changed the title Clarify the need of dependency to Spring Clarify the need for Spring WebFlux Apr 13, 2018
@bclozel
Copy link

bclozel commented Apr 13, 2018

I agree with @snicoll here - WebClient is not designed to be distributed as an independent HTTP client library, but rather a piece of the WebFlux ecosystem. In comparison, the cf-java-client has chosen to directly use reactor-netty just for that.

@michaelklishin
Copy link
Member

So what are you saying, that we should not use WebClient? The rationale seems to be a poor design decision on the Spring Boot side. What should we use instead if we don't want to reinvent an asynchronous HTTP client?

@bclozel
Copy link

bclozel commented Apr 13, 2018

Using or not WebClient is your choice ultimately - we're joint pointing to the fact that Spring Framework bundled WebClient and the web framework in spring-webflux because WebClient is not meant to be used independently as a client library.

Spring Boot is adapting to that and auto-configures things given the presence of spring-webflux. The fact that hop depends on a web framework is the problem here.

In my previous comment, I mentioned cf-java-client, which is using reactor-netty directly. cf-java-client is basically doing exactly that, so not only you don't need to reinvent a client (reactor-netty provides one, this is the one being used by WebClient under the covers) but also cf-java-client should do have the kind of implementation that you'd have to write anyway. If anything, this would make this library lighter and less prone to dependency conflicts.

@acogoluegnes
Copy link
Contributor

@snicoll HOP is a client for RabbitMQ Management HTTP API, usually used for monitoring reasons. It's not meant to be used for messaging protocols that RabbitMQ supports (AMQP, STOMP, etc). So the rationale here is just to use WebClient.

@bclozel This is far from obvious when looking at the Spring reference documentation: https://docs.spring.io/spring/docs/5.0.5.RELEASE/spring-framework-reference/web-reactive.html#webflux-client

Quoting the documentation:

The RestTemplate is not a good fit for use in non-blocking applications, and therefore Spring WebFlux application should always use the WebClient. The WebClient should also be preferred in Spring MVC, in most high concurrency scenarios, and for composing a sequence of remote, inter-dependent calls.

So the WebClient seemed to be the natural choice to port HOP synchronous, RestTemplate-based API to a reactive API.

If I get the dependency explanation correctly, you should maybe add a callout in the documentation, saying WebClient isn't meant to be used outside of a Spring WebFlux application and one should use Reactor Netty instead for standalone usage.

Beside that, I don't see how to help you here. Where do you pull Hop from? Spring AMQP I guess? We can plan to use Reactor Netty to refactor the ReactiveClient in HOP 3.0, but it's unlikely to happen in the next few days. Tell us more about what you're doing to see if we can find an easy workaround.

@garyrussell
Copy link
Contributor

garyrussell commented Apr 13, 2018

I had a discussion about this with @artembilan the other day (I can't find the dialog), but we concluded that we should probably make the http-client dep optional since using the management template is rather a corner case and users will have to opt-in. spring-rabbit currently doesn't use the webflux client (but we might consider adding it).

Unless, boot can exclude the transitive dep (since it won't - currently - affect spring-rabbit).

@acogoluegnes
Copy link
Contributor

@garyrussell Thanks for the clarification. Any idea when you'll use the reactive client (so we can start the refactoring to Reactor Netty)?

@garyrussell
Copy link
Contributor

@acogoluegnes We haven't decided yet (discussion here spring-projects/spring-amqp#736 - which is what I couldn't find earlier).

It will likely be in our 2.1 release (later in the summer).

@bclozel
Copy link

bclozel commented Apr 13, 2018

@acogoluegnes I agree we should add something in the documentation and make that more clear.
It's perfectly fine to only use WebClient in a Spring application that's not a web app, but I think it should not be consumed by libraries that don't want to have a full web framework on their classpath.

@acogoluegnes
Copy link
Contributor

OK, I suggest the following steps:

  • HOP 2.1: implement new reactive client based on Reactive Netty, deprecate ReactiveClient (Spring WebFlux-based), make Spring WebFlux dependency optional. Spring AMQP 2.1 could use this version.
  • HOP 3.0: remove ReactiveClient.

WDYT @michaelklishin @garyrussell?

@michaelklishin
Copy link
Member

We are a tiny team and it's very unfortunate that we have to throw away the existing client and redo all that work. @acogoluegnes that schedule SGTM.

@artembilan
Copy link
Contributor

Pay attention, please, that you also have a spring-web mandatory dependency for the standard client.
As far as we have a reactive variant this one should become optional as well.
Although if we talk here from the library perspective that sounds like no Spring dependencies should be here at all. Not sure how is it going to be complicated to get rid of Spring Web in favor of plain Apache HTTP Client, but we may consider to deprecate standard client as well in favor of a Reactive one. Since any blocking operations can easily be achieved with Flux and Mono blocking hooks.

acogoluegnes added a commit that referenced this issue Apr 24, 2018
This commit covers the typical use cases (basic authentication, serialization,
deserialization, GET/PUT/DELETE methods). Some refactoring will be
needed to re-use and clean up code (e.g. URI creation, authentication).
TLS support will also be needed.

[#157001487]

References #122
@acogoluegnes acogoluegnes added this to the 2.1.0 milestone Apr 24, 2018
acogoluegnes added a commit that referenced this issue Apr 24, 2018
@rstoyanchev
Copy link

I would rather say the expectation is that where RestTemplate and WebClient are used, Spring Framework dependencies (spring-core, spring-context, spring-web) are already on the classpath. Or otherwise you don't mind having those dependencies.

If HOP is currently using the RestTemplate, then the decision to use the WebClient seems perfectly reasonable to me. The WebClient does provide a higher level API, a set of codecs, and pluggable support for HTTP clients (Jetty, and JDK HttpClient in the works).

From the comments under spring-projects/spring-boot#12853 I see that there is a way to indicate a non-web environment. Forgive me if this is a naive question but I'm wondering if there is room for improvement in how a decision is made whether to start a WebFlux server or not? Is there some way to know if spring-boot-starter-webflux is declared or not?

@bclozel
Copy link

bclozel commented Apr 25, 2018

A Spring Boot starter is nothing but a convenient list of dependencies. In this case, spring-boot-starter-webflux is roughly spring-webflux + reactor-netty, which both ship client and server implementations in a single jar.

Enforcing different behaviors between using starters and raw dependencies would be inconsistent with the whole Spring Boot ecosystem. Only a few options available here:

  1. Spring Framework repackages the WebClient (see SPR-16760), which is highly unlikely because it means relocating packages or potentially breaking jigsaw
  2. Spring Boot changes the default behavior for Spring WebFlux applications after the major release, making things inconsistent with everything else
  3. Spring Framework acknowledges in its documentation that WebClient is not meant to be distributed as an independent HTTP client library (which we've already done in this issue)

Judging from the commits beings prepared as we speak, HOP has alreday chosen to move to reactor-netty, so let's continue this discussion in SPR-16760.

@acogoluegnes
Copy link
Contributor

Thanks @rstoyanchev for clarification.

@bclozel We may keep the WebClient implementation and make the dependencies optional (#124). Users will just have to add the appropriate dependencies depending on the client they pick. We still plan to implement a client based on Reactor Netty though, it shouldn't be too much work.

@michaelklishin
Copy link
Member

I vote for complete removal of the WebFlux client. Having 3 clients in one library is too much. If we have no choice but to replace WebFlux with something that will be the "async default", we might as well make that thing the only option available.

@acogoluegnes
Copy link
Contributor

@michaelklishin I agree, maintaining 3 clients is a substantive effort. My concern now is to provide an appropriate solution to people using HOP in a Spring WebFlux server application, e.g. to share resources between different WebClient instances. If we can easily do that with Reactor Netty HttpClient, I don't mind getting rid of HOP WebClient implementation. WDYT @rstoyanchev?

@michaelklishin
Copy link
Member

@bclozel do you have a Spring doc link where it "acknowledges in its documentation that WebClient is not meant to be distributed as an independent HTTP client library"? From where I sit WebFlux is promoted heavily as a new Spring ecosystem feature. That's why we have adopted it.

@michaelklishin
Copy link
Member

@acogoluegnes unfortunately we are in a "damned if we do [integrate with WebFlux], damned if we don't" kind of situation here. This library was first and foremost developed as a standalone one, not a Spring project dependency. It should therefore be as minimalistic as possible. All the nice integration bells and whistles should either be on the Spring side or maintained as separate optional projects.

Hop was never meant to be a multi-month active development effort for our team. One way to meet that goal is to make it as small as possible and let other projects provide various integration bits.

acogoluegnes added a commit that referenced this issue Apr 30, 2018
This provide an abstraction and a reactive solution to create the token.
Default is to compute the basic value on demand and cache it.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 2, 2018
URI path encoding implementation is taken from HTTP components, this
avoids depending on Spring Web. The HTTP response wrapper contains the
status code, reason phrase, and headers, this avoids leaking the Reactor
Netty API.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 2, 2018
Namely channels, virtual hosts, users, permissions.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 3, 2018
Exchanges, bindings, shovel, queues.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 3, 2018
acogoluegnes added a commit that referenced this issue May 3, 2018
Exchanges & bindings.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 3, 2018
acogoluegnes added a commit that referenced this issue May 3, 2018
[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
This commit covers the typical use cases (basic authentication, serialization,
deserialization, GET/PUT/DELETE methods). Some refactoring will be
needed to re-use and clean up code (e.g. URI creation, authentication).
TLS support will also be needed.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
acogoluegnes added a commit that referenced this issue May 15, 2018
acogoluegnes added a commit that referenced this issue May 15, 2018
acogoluegnes added a commit that referenced this issue May 15, 2018
This provide an abstraction and a reactive solution to create the token.
Default is to compute the basic value on demand and cache it.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
URI path encoding implementation is taken from HTTP components, this
avoids depending on Spring Web. The HTTP response wrapper contains the
status code, reason phrase, and headers, this avoids leaking the Reactor
Netty API.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
Namely channels, virtual hosts, users, permissions.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
Exchanges, bindings, shovel, queues.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
acogoluegnes added a commit that referenced this issue May 15, 2018
Exchanges & bindings.

[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
acogoluegnes added a commit that referenced this issue May 15, 2018
[#157001487]

References #122
This was referenced May 15, 2018
acogoluegnes added a commit that referenced this issue May 15, 2018
[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
acogoluegnes added a commit that referenced this issue May 15, 2018
[#157001487]

References #122
acogoluegnes added a commit that referenced this issue May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants