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

Support for registering handlers in RSocketRequester.Builder #23170

Closed
rstoyanchev opened this issue Jun 21, 2019 · 7 comments
Closed

Support for registering handlers in RSocketRequester.Builder #23170

rstoyanchev opened this issue Jun 21, 2019 · 7 comments
Assignees
Labels
in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement
Milestone

Comments

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 21, 2019

Currently client-side handling involves registering an acceptor, for example like so, which is likely an RSocketMessageHandler bean with manually registered handlers.

Arguably in this case there isn't much reason for applications to be aware of the RSocketMessageHandler nor to have it as a bean, so we should offer an option to register such client-side handlers directly with RSocketRequester.Builder, which would internally create the RSocketMessageHandler and configure it as an acceptor in the RSocketFactory.

In the Javadoc we should make it clear that registering handlers is a shortcut for creating an RSocketMessageHandler and configuring it as an acceptor. That latter still remains an option for more advanced cases and that should be made clear.

@rstoyanchev rstoyanchev added in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement labels Jun 21, 2019
@rstoyanchev rstoyanchev added this to the 5.2 RC1 milestone Jun 21, 2019
@bclozel bclozel self-assigned this Jun 21, 2019
@bclozel
Copy link
Member

bclozel commented Jun 21, 2019

I'm working on this right now.
There's one downside to introducing a new method on the RSocketRequester.Builder like this:

RSocketRequester.Builder handlers(Object... handlers);

Annotated handlers cannot be typed (they're like MVC controllers). In #23135 we're discussing the possibility to introduce a functional variant for RSocket handlers. In this case they would have a proper type, and we would probably need to configure them on the builder as well.

Is there a way to design annotated handlers being configured on the requester, without clashing with potential future plans for the functional variant?

@rstoyanchev
Copy link
Contributor Author

The method could be called annotatedHandlers leaving room for alternative handlers (and type of infrastructure) in the future.

@bclozel
Copy link
Member

bclozel commented Jun 24, 2019

This commit introduced a package tangle by referencing RSocketMessageHandler (from the annotation package) in the main rsocket package.

We should reconsider the way to solve this issue, as this circular dependency when/if we try to introduce the function handlers variants here. Functional handlers will be typed so it will be even more obvious.

@bclozel bclozel reopened this Jun 24, 2019
@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Jun 24, 2019

Object... for handlers is convenient but also inflexible. For any type of customization you would need to fall back on declaring an RSocketMessageHandler bean.

One idea would be to create a ClientResponderBuilder that RSocketMessageHandler provides an implementation for. Something like this:

ClientResponderBuilder responderBuilder =
        RSocketMessageHandler.clientResponderBuilder(new ClientHandler())
                .routeMatcher(routeMatcher)
                .build();

RSocketRequester.builder()
        .responder(responderBuilder)
        .rsocketStrategies(rsocketStrategies)
        .connectTcp("localhost", server.address().getPort())

This is slightly less convenient but more flexible and also breaks the package cycle if ClientResponderBuilder lives in the top-level rsocket package.

bclozel added a commit to bclozel/spring-framework that referenced this issue Jul 8, 2019
Prior to this commit, the `RSocketRequester.Builder` would allow to
configure directly annotated handlers for processing server requests.
This lead to a package tangle where the `o.s.messaging.rsocket` would
use classes from `o.s.messaging.rsocket.annotation.support` package.

This commit introduces the `ClientResponder` interface for configuring a
responder on the client requester. This is compatible with future
changes with a parallel implementation of a functional variant for
RSocket handlers.

Closes spring-projectsgh-23170
@bclozel
Copy link
Member

bclozel commented Jul 8, 2019

@rstoyanchev I've got a slightly different proposal for this: bclozel@f6b879d

The Builder interface didn't feel right as we're building a client acceptor for the requester (and that's the only common interface). RSocket doesn't provide a dedicated interface for now. Happy to discuss further changes for this.

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Jul 13, 2019

It's true RSocket doesn't provide an interface for a "client acceptor" which is actually a misnomer since on the client side it's merely a callback before the connection is even established. I'm going to rename the clientAcceptor and serverAcceptor in RSocketMessageHandler to clientResponder and serverResponder.

Next to that the proposed clientResponder(Object...) static method is reallyclientResponderFactory(Object...) returning ClientResponderFactory. However as a shortcut, this factory method is limited and doesn't allow customization of further RSocketMessageHandler properties. I see RSocketRequestBulider exposes a RouteMatcher but there is no viable path for exposing any further RSocketMessageHandler properties. That would be limiting then if you have to go back to creating RSocketMessageHandler for any other customization.

Back to the original idea, I think the main goal is to allow creating an RSocketRequester programmatically including some way to build an instance of RSocketMessageHandler without having to declare it as a Spring bean. RSocketMessageHandler is overwhelming for direct use with a number of options not applicable to programmatic, direct registration of handlers. Focusing on that, I would prefer to see something that effectively builds an RSocketMessageHandler instance and helps to configure a client responder.

So perhaps a ClientResponderFactory interface with a create(Object...) default method. That methods returns a ClientResponderFactory instance that is ready to pass to RSocketRequester.Builder as is, but also exposes further config methods for the subset of RSocketMessageHandler properties focusing on programmatic use (outside of Spring config) and relevant to direct registration of handlers.

artembilan added a commit to spring-projects/spring-integration that referenced this issue Jul 14, 2019
@bclozel
Copy link
Member

bclozel commented Jul 16, 2019

I gave another attempt at this and I'm still not convinced for the following reasons:

  1. Creating a default method on the ClientResponderBuilder re-introduces a package dependency to org.springframework.messaging.rsocket.annotation.support, since the default implementation would have to rely on RSocketMessageHandler somehow
  2. Most of the options available on RSocketMessageHandler (RSocketStrategies, MetadataExtractor, default MimeType) are already configured on the RSocketRequester or use types from org.springframework.messaging.rsocket.annotation.support; there's then a conflict between reusing the ones configured in the requester and checking whether developers would like to override some. I don't see an instance of why we would have a different configuration for those on the requester and responder, or at least one that's worth introducing many public methods.
  3. From an auto-configuration perspective, we should ensure that Spring Boot can auto-configure an RSocketRequester.Builder and that configuring a responder on it doesn't require fetching additional beans from the context
  4. Even if the functional variant of RSocket handlers is not there, I'm a bit worried about introducing ClientResponderBuilder.create(Object...) as it would not leave us that much space if we need to introduce a way to register functional handlers there

rstoyanchev added a commit that referenced this issue Jul 18, 2019
The new interface supersedes ClientResponderFactory and is more general,
for any RSocketFactory customization.

DefaultClientResponderFactory implements the new interface and is
renamed to AnnotationClientResponderConfigurer.

See gh-23170
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants