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

Use @MessageMapping method signature to constrain RSocket interaction model #23999

Closed
bclozel opened this issue Nov 14, 2019 · 1 comment
Closed
Assignees
Milestone

Comments

@bclozel
Copy link
Member

@bclozel bclozel commented Nov 14, 2019

Currently, our RSocket support is mapping REQUEST_FNF, REQUEST_RESPONSE, REQUEST_STREAM and REQUEST_CHANNEL on @MessageMapping annotated handler methods on one side, SETUP and METADATA_PUSH on @ConnectMapping on the other.

Context of the issue

The case of "fire and forget" requests, where the requester sends a request and does not expect any response, can be interesting here in the context of our flexible signature support and the current mapping process.

While looking at our RSocket support with @snicoll, we considered a Controller handler that returns a response stream:

@MessageMapping("test")
public Flux<Long> test() {
	return Flux.interval(Duration.ofSeconds(1));
}

If a requester calls this handler with a fire and forget interaction type:

Mono<Void> done = requesterBuilder.dataMimeType(MediaType.APPLICATION_CBOR)
		.connectTcp("localhost", this.localPort)
		.flatMap(req -> req.route("test").send());

Then we're getting the following exception on the server side:

java.lang.IllegalArgumentException: Missing 'rsocketResponse'
	at org.springframework.util.Assert.notNull(Assert.java:198)
	at org.springframework.messaging.rsocket.annotation.support.RSocketPayloadReturnValueHandler.handleEncodedContent(RSocketPayloadReturnValueHandler.java:65)
	at org.springframework.messaging.handler.invocation.reactive.AbstractEncoderMethodReturnValueHandler.lambda$handleReturnValue$0(AbstractEncoderMethodReturnValueHandler.java:124)
	at org.springframework.messaging.handler.invocation.reactive.ChannelSendOperator$WriteBarrier.onNext(ChannelSendOperator.java:194)

In this particular case (and in general), we can find pairs of handler definitions / rsocket interactions that mismatch in our mapping infrastructure. For example, a request can be mapped to a handler, the handler is executed but the returned subscriber is not subscribed to.

After chatting with @rstoyanchev, we thought that we should enforce a few design choices in our implementation.

Refining message mappings with handler return types

First, when mapping requests on handlers, returning more elements than expected should not be allowed, but returning less than expected should be permitted.
For example:

  • a REQUEST_FNF should only be mapped on handlers that return Mono<Void> or void. Other types (even Mono<?>) should not be considered for mapping.
  • a REQUEST_RESPONSE should only be mapped on Mono types, as Flux might return more than expected. In this case, we could automatically take(1) on the returned publisher, but this goes against the general behavior we've established in reactor and webflux so far.

Not allowing ambiguous mappings

Even in the light of this (restricting message mapping depending on the handler return type), we should not allow multiple handlers with the same route. If this happens, this will be treated as an error since the mapping is ambiguous.

@rstoyanchev rstoyanchev changed the title Restrict RSocket message mapping depending handler return type Add constraints on @MessageMapping method return values types based on RSocket interaction model Nov 14, 2019
@rstoyanchev rstoyanchev added this to the 5.2.2 milestone Nov 14, 2019
@rstoyanchev rstoyanchev changed the title Add constraints on @MessageMapping method return values types based on RSocket interaction model Use @MessageMapping method signature to constrain RSocket interaction model Nov 18, 2019
@rstoyanchev

This comment has been minimized.

Copy link
Contributor

@rstoyanchev rstoyanchev commented Nov 18, 2019

The way this came out in the end is that @ConnectMapping methods are validated on startup and rejected unless they return void or Mono<Void>.

For @MessageMapping methods, the conditions are refined based on the declared input and output cardinality of the method. That means a method won't even match based on the route alone. It must also be a good fit for the request interaction type. In case of no match we'll do an extra pass to find partial matches and reply with a helpful error that points to the supported interaction type. As one positive consequence, a request-response and a request-stream can now match independently for the the same route which previously would have been an ambiguous mapping.

See the changes in the reference docs for more details.

artembilan added a commit to spring-projects/spring-integration that referenced this issue Nov 19, 2019
Related to spring-projects/spring-framework#23999

* Since Spring Integration inbound endpoints are generic in their method
signature we can't rely on a new `EMPTY_CONDITION` because it turns on
configuration merge into just `FrameType.REQUEST_FNF` &
`FrameType.REQUEST_RESPONSE`.
So, use `FrameType.REQUEST_FNF`, `FrameType.REQUEST_RESPONSE`,
`FrameType.REQUEST_STREAM` & `FrameType.REQUEST_CHANNEL` explicitly to
cover all the valid request-response models for SI endpoints
* Rework `RSocketOutboundGatewayIntegrationTests` according new logic.
Plus refactor to earlier subscription into `FluxMessageChannel` to
avoid potential race conditions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.