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

Consistent error handling when streaming with servlet vs reactive stack [SPR-17440] #21972

Closed
spring-projects-issues opened this issue Oct 28, 2018 · 3 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Oct 28, 2018

Michal Domagala opened SPR-17440 and commented

Consider the following code:

@RequestMapping("/1")
Flux<String> v1()
    return Flux.error(new ResponseStatusException(BAD_REQUEST));

@RequestMapping("/2")
Flux<String> v2()
    return Flux.concat(Flux.just("x"),Flux.error(new ResponseStatusException(BAD_REQUEST)));

 
In reactive stack initial exception has special treatment: class ChannelSendOperator has special state NEW: /** No emissions from the upstream source yet. */

Due special treatment http response status for /1 is 400 and for /2 is 200
In servlet stack http response status for both endpoints is 200

Nice to have consistent implementation. Reactive stack way is correct one.


Affects: 5.0.10

Reference URL: https://stackoverflow.com/questions/53013088/how-to-peep-if-flux-has-exception

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 29, 2018

Rossen Stoyanchev commented

Indeed the asynchronous, streaming support in Spring MVC (e.g. SseEmitter, ResponseBodyEmitter) was created first and it's always behaved as it does now, i.e. flushing the status and headers before any items are emitted. The solution in WebFlux went further, and it had to, because async is the norm there and it's the only way for @ExceptionHandler to be supported. We should try and make the two consistent.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 30, 2018

vrawat79 commented

Actually, since we are returning a Flux, i.e. a series of events, wherein each event could have its own status, shouldn't we be returning a HTTP 207 as the overall response code.

400 as the overall status code will be mis-leading. 

 

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 30, 2018

Rossen Stoyanchev commented

The ResponseBodyEmitter (and SseEmitter) were designed for a stream of events (like SSE) and that's very different. The SSE spec has no reference to HTTP status 207 and I've never seen that status used for such purposes. We're only talking about the initial status before the stream actually begins emitting. From what I can see HTTP 207 is defined in WebDAV and that's a very specific thing for multiple operations or resources.

@spring-projects-issues spring-projects-issues added this to the 5.2 RC1 milestone Jan 11, 2019
@rstoyanchev rstoyanchev removed their assignment Jan 28, 2019
@rstoyanchev rstoyanchev changed the title Initial exception Flux should be handled the same way in servlet stack as in reactive stack [SPR-17440] Consistent error handling when streaming with servlet vs reactive stack [SPR-17440] Jan 29, 2019
@jhoeller jhoeller modified the milestones: 5.2 M1, 5.2 M2 Mar 23, 2019
@rstoyanchev rstoyanchev self-assigned this Apr 19, 2019
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
3 participants