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

RSocket Auto-Configuration: Make RSocketWebSocketNettyRouteProvider public #18549

Closed
FWinkler79 opened this issue Oct 9, 2019 · 7 comments
Closed
Assignees

Comments

@FWinkler79
Copy link

Used Spring Versions

Used Spring Boot Version: 2.2.0.BUILD-SNAPSHOT
Used Spring Version: 5.2
User Spring Cloud Version: Hoxton.M2
Sample Project: https://github.com/FWinkler79/SpringCloudPlatform/tree/Issue-SpringBootRsocketAutoconfig

What's the issue?

RSocketWebSocketNettyRouteProvider is package protected and is declared in RSocketServerAutoConfiguration#WebFluxServerAutoConfiguration as an overridable bean named rSocketWebsocketRouteProvider.

I would like to override that bean, since for now this is the only reasonable way to configure the ServerRSocketFactory to add things like lease handling. See this stackoverflow question which describes the reason why I need to do it like that currently.

In order to override the rSocketWebsocketRouteProvider bean, I either need to place my bean configuration in the same package as RSocketWebSocketNettyRouteProvider (which then might cause all kinds of component scan issues) or I need to create a subclass of it, in the same package (which is my current solution).

Generally, it is tedious having to do so, and could be avoided, if the RSocketWebSocketNettyRouteProvider were a public class.

Expected Solution

  1. Make RSocketWebSocketNettyRouteProvider public.
  2. Make it possible for an application to define an arbitrary number of @Ordered ServerRSocketFactoryCustomizer beans that will be picked up to modify the auto-configured ServerRSocketFactory (as described on stack overflow)
  3. Consider exposing ServerRSocketFactory (created in RSocketWebSocketNettyRouteProvider#apply(...)) as a bean. That would allow applications to easily add additional configurations to it.

Sample Project

You can find a sample project that shows the current solution here.

To get it started, proceed as follows:

  1. start Zipkin (using scripts/startZipkin.sh
  2. start RabbitMQ (using scripts/startRabbit.sh
  3. start service-registry
  4. start config-server
  5. start rsocket-server
  6. start rsocket-client

Notice class CustomRSocketWebSocketNettyRouteProvider in rsocket-server which is a subclass of RSocketWebSocketNettyRouteProvider and that it needs to be declared in a Spring Boot auto-config package.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 9, 2019
@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Oct 14, 2019
@bclozel
Copy link
Member

bclozel commented Oct 14, 2019

I've read your StackOverflow question and the former ServerRSocketFactoryCustomizer is not there anymore, and it is known as ServerRSocketFactoryProcessor (and is not a Function anymore). See #18395.

I guess this should solve the root issue, and it's probably the best way to achieve that since this processor is applied for both standalone RSocket servers and Spring WebFlux + RSocket servers. In the case of RSocketWebSocketNettyRouteProvider, this is only applied to WebFlux servers.

Now we need to reconsider this still - you can implement and add already as many NettyRouteProvider to your context and they will be processed. In the case of RSocketWebSocketNettyRouteProvider, I'm not sure we should make that type public and in that case, we might remove the @ConditionalOnMIssingBean annotation.

Could you try the same with our latest 2.0.0.RC1 version and let me know if that works?

@bclozel bclozel self-assigned this Oct 14, 2019
@FWinkler79
Copy link
Author

Hi there, I don't see why the referenced issue is related. Is the reference actually correct?
All I see done in the issue is a change of tomee version.

And version 2.0.0.RC1 of which library are you talking about?
I am using:
Spring Boot Version: 2.2.0.BUILD-SNAPSHOT
Spring Version: 5.2
Spring Cloud Version: Hoxton.M2

I am a bit puzzled...

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 15, 2019
@wilkinsona
Copy link
Member

I think @bclozel intended to reference #18390 and meant to ask you to try with Spring Boot 2.2.0.RC1.

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 15, 2019
@FWinkler79
Copy link
Author

Thanks @wilkinsona and @bclozel.
I tried version 2.2.0.RC1 of Spring Boot and changed my code to use ServerRSocketFactoryProcessor instead. Now it works nicely!
For those interested, the sample that shows it is here.
Thanks again, for the quick resolution!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 16, 2019
@wilkinsona
Copy link
Member

Great stuff. Thanks very much for taking the time to try it.

@wilkinsona wilkinsona removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Oct 16, 2019
@FWinkler79
Copy link
Author

Just one more little thing for consideration: In NettyRSocketServerFactory the method to set the ServerRSocketFactoryProcessors is still called setServerProcessors(...), maybe a better name would be setServerFactoryProcessors(...).

@wilkinsona
Copy link
Member

Good catch, @FWinkler79. Thank you. I've opened #18617.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants