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.

Sign up for GitHub

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

Jump to bottom

Improvements to RSocketStrategies #23314

Closed
bclozel opened this issue Jul 18, 2019 · 3 comments
Closed

Improvements to RSocketStrategies #23314

bclozel opened this issue Jul 18, 2019 · 3 comments
Assignees
@rstoyanchev
Labels
in: messaging Issues in messaging modules (jms, messaging) in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone
5.2 RC1

Comments

@bclozel
Copy link
Member

bclozel commented Jul 18, 2019

We should try and use the PathPatternRouteMatcher as a default RouteMatcher implementation in the RSocket infrastructure if it is available (i.e. if spring-web is on the classpath).
@bclozel bclozel added the type: enhancement A general enhancement label Jul 18, 2019
@bclozel bclozel added this to the 5.2 RC1 milestone Jul 18, 2019
@bclozel bclozel self-assigned this Jul 18, 2019
@bclozel bclozel mentioned this issue Jul 18, 2019
Closed
@bclozel
Copy link
Member Author

bclozel commented Jul 18, 2019

Here's a summary of the current situation:

  • spring-messaging does not depend on spring-web
  • spring-web provides an efficient implementation of RouteMatcher, used with Spring WebFlux: PathPatternRouteMatcher
  • we'd like to use it if available in RSocket, if spring-web is on the classpath
  • in the long term, we might promote the PathPattern infrastructure as a default in other places (like Spring MVC)

We could add an optional dependency from spring-messaging to spring-web, but that would break our strict module arrangement and could lead to unfortunate imports. We could still avoir those imports with custom checkstyle rules.

We could use a 100% reflection approach for this case, but one could argue that we're usually not in the business of conditional configuration for Spring dependencies (usually Spring Boot is in charge of that).

The third alternative would be to move that PathPattern infrastructure to core, but this is even more disruptive at that point.

Flagging this for team attention.

@bclozel bclozel removed this from the 5.2 RC1 milestone Jul 18, 2019
@rstoyanchev
Copy link
Contributor

Team Decision: Add a small number of extensions in spring-web under org.springfamework.web.remoting.rsocket to make it possible to plug in the PathPatternRouteMatcher and codecs available in spring-web. Probably a couple of customizers for RSocketMessageHandler and RSocketStrategies.

@rstoyanchev rstoyanchev added in: messaging Issues in messaging modules (jms, messaging) in: web Issues in web modules (web, webmvc, webflux, websocket) and removed for: team-attention labels Jul 22, 2019
@rstoyanchev rstoyanchev assigned rstoyanchev and unassigned bclozel Jul 22, 2019
@rstoyanchev rstoyanchev changed the title Use PathPatternRouteMatcher with RSocket if spring-web is on classpath Support for RSocket infrastructure pre-configured with spring-web facilities Jul 23, 2019
rstoyanchev added a commit that referenced this issue Jul 24, 2019
@rstoyanchev
Updates to RSocket[Strategies|Requester] defaults
a780cad 
1. RSocketStrategies hooks in the basic codecs from spring-core by
default. Now that we have support for composite metadata, it makes
sense to have multiple codecs available.

2. RSocketStrategies is pre-configured with NettyDataBufferFactory.

3. DefaultRSocketRequesterBuilder configures RSocket with a frame
decoder that matches the DataBufferFactory choice, i.e. ensuring
consistency of zero copy vs default (copy) choice.

4. DefaultRSocketRequesterBuilder now tries to find a single non-basic
decoder to select a default data MimeType (e.g. CBOR), or otherwise
fall back on the first default decoder (e.g. String).

See gh-23314
rstoyanchev added a commit that referenced this issue Jul 24, 2019
@rstoyanchev
Add responder strategies to RSocketStrategies
91b040d 
RouteMatcher and MetadataExtractor can now be configured on and
accessed through RSocketStrategies. This simplifies configuration for
client and server responders.

See gh-23314
rstoyanchev added a commit that referenced this issue Jul 24, 2019
@rstoyanchev
Add create shortcut to RSocketStrategies
c456950 
Now that RSocketStrategies has default settings it makes sense to have
a create() shortcut vs builder().build().

This commit also updates tests to take advantage of improvements in this
and the previous two commits.

See gh-23314
rstoyanchev added a commit that referenced this issue Jul 24, 2019
@rstoyanchev
Simplify RSocket client responder config
e19e36a 
Now that responder RSocketStrategies also exposes responder strategies,
AnnotationClientResponderConfigurer is reduced and no longer needs to
be public. This commit folds it into RSocketMessageHandler as a nested
class and exposes it as a ClientRSocketFactoryConfigurer through a
static method that accepts the handlers to use.

Effectively a shortcut for creating RSocketMessageHandler, giving it
RSocketStrategies, calling afterPropertiesSet, and then the instance
createResponder.

See gh-23314
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jul 24, 2019 •
edited
Loading

In the end this turned into a series of improvements in spring-messaging.

  • Improved defaults in RSocketStrategies, e.g. registration of basic codecs from spring-core
  • Exposing RouteMatcher and MetadataExtractor through RSocketStrategies
  • Folding what was left of AnnotationClientResponderConfigurer after the changes into it a private nested class within RSocketMessageHandler and exposing it as a static method there

See commit messages for the above commits for more details.

@rstoyanchev rstoyanchev added this to the 5.2 RC1 milestone Jul 24, 2019
@rstoyanchev rstoyanchev changed the title Support for RSocket infrastructure pre-configured with spring-web facilities Improvements to RSocketStrategies Jul 24, 2019
bclozel added a commit to spring-projects/spring-boot that referenced this issue Jul 24, 2019
@bclozel
Update RSocket configuration after Framework changes
1996952 
Since spring-projects/spring-framework#23314, the `RSocketStrategies`
provide more codecs by default, and there is no need to order them to
avoid conflicts during mime type selection.

This commit also ensures that the `PayloadDecoder.ZERO_COPY` is
configured on the RSocket server if the configured `DataBufferFactory`
is compatible with that strategy.
bclozel added a commit to spring-projects/spring-boot that referenced this issue Jul 24, 2019
@bclozel
Reinstate RSocket codecs configuration ordering
5beb7ed 
Since spring-projects/spring-framework#23314 and the following commit
spring-projects/spring-framework@be4facef1b, the RSocket codec selection
is relaxed and the order of configured commits matters again.

This commit ensures that the CBOR codec is configured ahead of the JSON
codec so that it can be chosen first if no data mime type is specified
when a connection is established with a client requester.
bclozel added a commit to spring-projects/spring-boot that referenced this issue Sep 20, 2019
@bclozel
Configure RouteMatcher at the RSocketStrategies level
070eeda 
After a change in Spring Framework (see
spring-projects/spring-framework#23314), the `RouteMatcher` to be used
with the RSocket infrastructure is configured on the `RSocketStrategies`
directly.

This commit moves the auto-configuration of the
`PathPatternRouteMatcher` from the message handling parts to the RSocket
strategy one.

Closes gh-17571
pull bot pushed a commit to scope-demo/spring-boot that referenced this issue Sep 20, 2019
@bclozel @pull
Configure RouteMatcher at the RSocketStrategies level
1d51f86 
After a change in Spring Framework (see
spring-projects/spring-framework#23314), the `RouteMatcher` to be used
with the RSocket infrastructure is configured on the `RSocketStrategies`
directly.

This commit moves the auto-configuration of the
`PathPatternRouteMatcher` from the message handling parts to the RSocket
strategy one.

Closes spring-projectsgh-17571
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rstoyanchev rstoyanchev

Labels
in: messaging Issues in messaging modules (jms, messaging) in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Milestone
5.2 RC1
Development

No branches or pull requests
2 participants
@bclozel @rstoyanchev