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

Handle streaming response errors #595

Merged
merged 9 commits into from
Nov 20, 2023
Merged

Handle streaming response errors #595

merged 9 commits into from
Nov 20, 2023

Conversation

jeremyg484
Copy link
Contributor

Streaming reactive response body processing is updated to complete the response when an error signal is encountered.

This brings parity to the way streaming errors are handled in the Netty server implementation. Previously an error
signal resulted in the response hanging and never completing.

This mostly fixes the TCK test failures encountered when trying to update to Micronaut Core 4.1.11 (#588), but one of the
test methods in the TCK's ErrorHandlerFluxTest will need to be updated to detect the error properly with
mid-stream errors and the way they get written to the servlet response.

@jeremyg484 jeremyg484 self-assigned this Nov 8, 2023
@jeremyg484
Copy link
Contributor Author

The necessary update to ErrorHanlderFluxTest is in micronaut-projects/micronaut-core#10082


then:
HttpClientResponseException ex = thrown(HttpClientResponseException)
ex.status == HttpStatus.OK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is 200 code expected for an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdelamo Yes, because this is an odd case. As the error occurs during a chunked response, the status code has already been written and can't be changed. The only thing the server can do is just stop writing the data and end the response. The expected behavior being verified is that incomplete data is sent and thus the client exception happens when actually trying to deserialize the response.

wetted and others added 6 commits November 10, 2023 11:23
…rtow/UndertowErrorSpec.groovy

Co-authored-by: Sergio del Amo <[email protected]>
…rtow/UndertowErrorSpec.groovy

Co-authored-by: Sergio del Amo <[email protected]>
…/TomcatErrorSpec.groovy

Co-authored-by: Sergio del Amo <[email protected]>
…/TomcatErrorSpec.groovy

Co-authored-by: Sergio del Amo <[email protected]>
Copy link

sonarcloud bot commented Nov 10, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

@wetted
Copy link
Contributor

wetted commented Nov 10, 2023

Oh, hell @jeremyg484 I committed Sergios' suggested commits. I thought htis was my servlet PR. I need to pay more attention. Happy Friday.😛

@jeremyg484
Copy link
Contributor Author

Oh, hell @jeremyg484 I committed Sergios' suggested commits. I thought htis was my servlet PR. I need to pay more attention. Happy Friday.😛

@wetted All good, thanks for the help (even if unintentional)! ;)

Copy link

sonarcloud bot commented Nov 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

88.9% 88.9% Coverage
0.0% 0.0% Duplication

@wetted
Copy link
Contributor

wetted commented Nov 10, 2023

Oh, hell @jeremyg484 I committed Sergios' suggested commits. I thought htis was my servlet PR. I need to pay more attention. Happy Friday.😛

@wetted All good, thanks for the help (even if unintentional)! ;)

🥴 I left one of Sergio's questions unresolved for you to answer.

@sdelamo sdelamo merged commit 3fe286c into master Nov 20, 2023
16 checks passed
@sdelamo sdelamo deleted the streaming-error-handling branch November 20, 2023 14:26
@sdelamo sdelamo added the type: bug Something isn't working label Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants