search

spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.32k stars 38.01k forks source link

RxJava Observable/Single and Reactor Flux/Mono as Spring MVC handler method return types [SPR-14046] #18618

Closed spring-projects-issues closed 7 years ago

spring-projects-issues commented 8 years ago

Spencer Gibb opened SPR-14046 and commented

UPDATE: This was resolved as "Won't Fix" in 4.x but has been fixed in 5.0 instead. See SPR-15365 for more details.

Spring Cloud Netflix has generated interest in supporting return value types of Observable and Single from RxJava. Reactor Core support would make sense as well.

Affects: 4.2.5

Reference URL: https://github.com/spring-cloud/spring-cloud-netflix/pull/778

Issue Links:

Referenced from: commits https://github.com/spring-projects/spring-framework/commit/92dd4eb3ef42fe3952ae87e67150554427e795b8, https://github.com/spring-projects/spring-framework/commit/971ccab038ad8e549d515a80e2d64c5d4c80c86b

0 votes, 8 watchers

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

Scheduling tenatatively for 4.3 RC1.

spring-projects-issues commented 8 years ago

Sébastien Deleuze commented

I would like to raise 2 points about this issue, based on some thoughts I had while working on Spring Reactive.

One point to keep in mind is that unlike for SSE or SockJS support where the client will receive multiple messages, for regular @ReponseBody the browser client will receive only one response (even if splitted in multiple chunks with HTTP/1.1, that's not visible from the client). As a consequence, for @ReponseBody I think serializing a RxJava Observable or a Reactor Flux should produce the same output than for a List. I highlight that because if we use ResponseBodyEmitter as discussed with Rossen Stoyanchev (that would be the best technical solution), the output would be {"foo1":"bar1"}{"foo2" :"bar2}, while the client expect [{"foo1":"bar1"},{"foo2" :"bar2"}] for non SSE HTTP response. The solution proposed in the PR is to use DeferredResult + Observable.toList() (so we do not take advantage of ResponseBodyEmitter). If we want to use ResponseBodyEmitter, a solution experimented in Spring Reactive is to insert for JSON use case a [ at the beginning of the response, a , between each result, and a ] at the end of the response. Given the fact that this solution would require more change, and is specific to each format supported, I think I would be in favor of keeping the DeferredResult + Observable.toList() solution for @ResponseBody in Spring Framework 4.3 (at least for format like JSON or XML, format like text could take advantage of ResponseBodyEmitter without requiring any special handling), while Spring Framework 5 will support real stream processing (serializing each element rather than the whole big thing) for Observable and Flux. For SSE ResponseBodyEmitter is perfectly suitable and should be used.

The other point I would like to raise is that if we want to support something more than using Observable + SseEmitter (currently possible as described in this blog post), it would be nice to support returning directly Observable or Flux for SSE method, rather than forcing users to return SseEmitter. But with the current behavior, I think the handler method must return SseEmitter to be handled as SSE. Maybe we should think about an alternative to qualify an handler method as a SSE one without using the return type. That could be:

The last solution avoids to introduce a new annotation or attribute, and is quite flexible (it allows to handle both SSE and Non SSE request with the same request handler method).

The goal would be to support handler methods like this for SSE:

@RequestMapping(method = RequestMethod.GET, value = "/messages")
public Observable<String> messages() {
    return Observable.just("message 1", "message 2", "message 3");
}

If users need more advanced option only supported by SseEmitter.SseEventBuilder, they could just use SseBuilder + Observable.

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

I have a PR in the works. From a programming model perspective it's support Single/Observable and Mono/Flux directly as return values. The trigger for SSE is the Content-Type response header which an additional shortcut on ResponseEntity can make short and sweet:

ResponseEntity.sse().body(observable);

Regarding whether the client expects a JSON stream or a JSON array, we are not making any changes whatsoever to what already exists and how it works. We are simply adapting Observable/Flux to ResponseBodyEmitter, nothing more. Today the ResponseBodyEmitter streams one POJO at a time and I don't see anything wrong with that. If anything to me the intent of returning Observable/Flux vs observable.toList() or flux.toList() has to be with streaming in mind. If we make an assumption the client can only handle an array we are effectively defeating the possibility of actual streaming. After all parsing a JSON stream is not that hard and there are JSON stream parsing libraries. The SockJS client is just one example of a library that does that but it also brings the point we can't make assumptions about what client is doing or even whether it's a browser client or not.

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

Here is the pull request https://github.com/spring-projects/spring-framework/pull/1005.

spring-projects-issues commented 8 years ago

Sébastien Deleuze commented

Rossen Stoyanchev +1 with SSE support with ResponseBodyEmitter, since the client is designed to receive each message independently and with and Single/Mono support.

But for regular HTTP @ReponseBody serialization of Flux and Observable, I still think it should be handled differently (or not supported at all) because {"foo1":"bar1"}{"foo2" :"bar2"} is not valid JSON and won't be parsed by browsers nor clients like RestTemplate, only [{"foo1":"bar1"},{"foo2" :"bar2"}] could be. Most HTTP clients (including browsers) are not designed for streaming (except for SSE designed for that use case). That's why both Spring Reactive and the PR serialize these types as an array. As you said it is possible to use libraries like Netty JsonObjectDecoder, but most clients will wait to have received the full response, defeating the stream processing. IMO, this is a very narrow use case not expected by 99% of users.

The ideal solution is the one supported by Spring Reactive/Spring Framework 5 : being able to perform stream processing of Flux and Observable while generating an array for format like JSON or XML that could be also parsed with libraries like JsonObjectDecoder. But since this is not possible with Spring Framework 4.3 without significant changes, I would favor having the same output in Spring Framework 4.3 and 5.0 for this use case, even if we are not processing it as a real stream for now (supporting it with DeferredResult will still be better that returning List<T> observable.toList().toBlocking().single() like users do currently. With the proper warning in the Javadoc, that make sense for Spring Framework 4.3 to have such limitation.

That would provide a behavior consistent with what users are expecting (almost all example I have seen work like that, including our own ones). If users want a real stream processing, they should use SSE (or wait Spring Framework 5).

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

Sébastien Deleuze, you can use ResponseBodyEmitter today to send POJOs and each one is serialized independently. So you get {"foo1":"bar1"}{"foo2" :"bar2"} although it's more accurate to think of it as {"foo1":"bar1"} first, then at any time later {"foo2" :"bar2"}, and so on. In other words either you say what we have today is wrong or otherwise Observable or Flux are nothing but a logical extension of it, a matter of trivial adapter code.

So let's discuss what we have today which I don't believe is wrong. The point of HTTP streaming is to produce many distinct messages as they become available. Call it a hack, or a deviation from HTTP, but it's re-using one HTTP response to stream what would otherwise be many responses over long polling. This is is similar to how WebSockets split a stream of bytes on one connection and exactly how SSE splits the HTTP response into events (but minus the formatting). In that sense thinking of the entire response as one document makes as much sense as it does in the case of SSE.

Now I get your point that SSE is the W3C standard way for browsers but purely from an API perspective ResponseBodyEmitter is a logical foundation for SSE support. In other words in order to support SSE, it makes sense to have a non-opinionated (with regards to formatting) foundation first. SSE then becomes nothing but formatting on top. It's just a common sense way of laying out the API so it allows for other types of formatting. For what it's worth this is what JAX-RS does too with their [ChunkedOutput|/https://jersey.java.net/documentation/latest/user-guide.html#chunked-output] and the SSE-based EventOutput on top.

Browses only support SSE but you can use JS libraries too. Admittedly I don't have anything more concrete on that but a quick search for "javascript http stream json" shows plenty of results and my sense is that HTTP streaming cases go well outside SSE and also well beyond the browser. The RestTemplate is not a great example since it does not provide first class support for HTTP streaming scenarios but as long as there is some agreement on a message format, it isn't hard to do. See our own RestTemplateXhrTransport that splits the response into a sequence of JSON array messages as expected by the SockJS protocol. We also have Netty and Jetty based implementations. You mentioned Netty's JsonObjectDecoder. Once you go in the direction of streaming messages do you really care if the whole thing is a valid JSON document and how does it help really? Even with an array, something, somewhere has to break it down, if what you're after is streaming of independent messages, and give you one array element at a time, in which case the "[" in the beginning and the "]" in the end become pointless. You aren't really consuming it as an array any more but rather as a stream of individual messages.

Last but not least I wonder if the differences come from thinking in terms of slightly different scenarios and concerns? One being HTTP streaming as in re-using the response to send many messages with arbitrary amounts of time in between individual messages (like WebSocket). Another being the streaming of a large document by breaking it up into smaller pieces that logically still form one response and are being sent without delay, i.e. as fast as client, server, and network can do.

spring-projects-issues commented 8 years ago

Sébastien Deleuze commented

Rossen Stoyanchev Thanks for your detailed feedback, I now get your point about HTTP streaming.

Indeed there are 2 different use cases for handling Flux and Observable for non SSE use cases:

Both are useful, so ultimately I think supporting both would make sense, but I don't think HTTP streaming should be the default behavior. Observable and Flux are not necessarily hot infinite streams. Let's imagine I implement a REST Webservice using MongoDB/Couchbase async driver or the upcoming Spring Data Reactive API like I did in spring-reactive-playground, in that very common use case I simply return Observable or Flux instead of Iterable or List. These types are just asynchronous non blocking collection returned by an API, but I still want to implement a valid REST webservice that will return valid JSON for regular HTTP clients like most Spring MVC application do today. With the Reactive support of Spring Framework 5, that will be just the default way to implement more scalable REST Webservices, but the output should remain the same than with Iterable or List.

Supporting HTTP streaming "à la Twitter" could be nice but IMO as an option, and I would rather advise users to use SSE for that use case since it is specifically built for that, allowing browsers to consume the stream "on the fly".

Maybe we could serialize Flux and Observable by default as a collection for @ResponseBody method (produce valid output, consistent with current behavior, perfect for the async non blocking collection use case), and support optionally HTTP Streaming with some flag like @ResponseBody(streaming=true)? But if we choose to support only one use case, I think it should be the regular HTTP with valid response bodies. To me enabling automatically HTTP streaming when an handler method return Flux or Observable is too much assumptions and will be very surprising and confusing for our users.

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

Both Observable and Flux have collection semantics built-in and I don't see much benefit in Spring MVC doing the same on your behalf:


@RequestMapping
public Observable<List<MyBean>>> handle() {
    // ...
    return observable.toList();
}

@RequestMapping
public Mono<List<MyBean>>> handle() {
    // ...
    return flux.toList();
}

To me the contract as it stands is very simple and easy to follow. You give an Observable or Flux, out comes a response where each emitted item is serialized with an HttpMessageConverter. It's also how it works today (as I mentioned) and we'd need to have some consistency.

While a Java 8 Stream return value translates well to collection semantics by default because that's mainly its purpose, with Observable and Flux it's a very different matter since they go far beyond collections and their main purpose is to support various sources. Given that "toList()" is so easy to call I don't see the downside. If the goal however is optimization, i.e. to serialize one item at a time and produce a collection response, then I think that should be measured first and treated as a case for case optimization. Not something that sets default behavior.

spring-projects-issues commented 8 years ago

Brian Clozel commented

Sorry for being late to this conversation, but I didn't have a strong opinion on this.

Going back to the original PR, it seems the main goal was to add more options for async/streaming use cases while embracing the Netflix OSS in Spring Cloud.

I do agree that the HTTP Streaming/SSE use cases sound right, but I have some concerns.In my opinion, there are conflicting interests here. On one hand, we want a clear message for our reactive web efforts, a strong Mono/Flux API, full RS support - basically helping developers to bridge that gap while keeping familiar MVC semantics. On the other hand, we want to properly support MVC use cases that are emerging from the community.

JSON over REST is still the biggest use case those days, and for this we'd provide "RxJava and Reactor support" in both cases, while meaning really different things. One really supports back pressure, the other won't and will deal with collections in a different way, buffering responses, etc. Is there a lack of consistency here?

Rather than helping developers to bridge that gap (if they need to), I think this is going to be really confusing and will work against us when developers try to convert a Spring MVC Controller to a reactive web one. The List<User> nicely translates to Flux<User> and really shows the rationale. For me, using the Flux/Mono API is one of the key differences that explains the reactive world.

So I am in favor of supporting this only for Streaming/SSE use cases, or not supporting this at all and restricting RxJava/Reactor support to reactive web.

spring-projects-issues commented 8 years ago

Rossen Stoyanchev commented

After some further discussion the general sense is that rx.Single translates well to DeferredResult and so does x.Observable to SseEmitter but the use of Observable for non-SSE scenarios cannot be supported easily and would likely cause more confusion than help. Furthermore it seems that most use cases today boil down to either a single value being emitted (including something like observable.toList().toSingle() or observable.toMap().toSingle()) or Server-Sent Events.

To keep things clear we've decided to only roll in a refactoring commit into 4.3 that further simplifies the ability to adapt alternative async types. Built-in support for RxJava and Reactor reactive types however is left for spring-reactive and Spring Framework 5 to provide. This pull request still has the RxJava commit that we've decided against.