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

Tck: Tests for error handlers with flux controller methods #10025

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

jeremyg484
Copy link
Contributor

@jeremyg484 jeremyg484 commented Oct 25, 2023

After investigating micronaut-projects/micronaut-reactor#238, I've got some questions to improve my understanding of how @Error handlers work in conjunction with @Controller methods that return a Flux, and try to figure out if there are any actual issues we need to address.

The test added by this PR exercises the existing behavior in various combinations of a Flux returning controller method in conjunction with an @Error handler method. It demonstrates the following:

A) A Flux returning controller method that is annotated with @SingleResult results in the @Error handler getting invoked as expected if the Flux signals an error - i.e., return Flux.error(new MyException())

B) A Flux returning controller method that throws an exception results in the @Error handler getting invoked regardless of whether or not the method is annotated with @SingleResult.

C) A Flux returning controller method that is not annotated with @SingleResult results in the @Error handler not getting invoked if the Flux signals an error as in scenario A.

D) A Flux returning controller method that is not annotated with @SingleResult that has an error signaled after the body has already begun to be written in the chunked response causes the connection to be closed and a warning logged without invoking the @Error handler as expected.

My questions:

  1. I would guess that the behavior of scenario C is intentional because it is a chunked response and it is possible that the headers have already been written. Is that guess correct?

  2. If so, would it be possible for us to treat a Flux that immediately signals an error (without any data events) differently? Could we invoke the error handler at that point, or is it too difficult because status and headers have already been written? 

  3. Is the behavior in the B scenario also intentional? It seems inconsistent to me that throwing an exception results in different behavior than signaling an error.

@jeremyg484 jeremyg484 changed the base branch from 4.2.x to 4.1.x October 25, 2023 21:50
@sdelamo
Copy link
Collaborator

sdelamo commented Oct 26, 2023

@jeremyg484 did you forget to push the code?

@jeremyg484
Copy link
Contributor Author

@jeremyg484 did you forget to push the code?

@sdelamo Fixed.

Copy link
Collaborator

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

The current assertions describe the current behavior, do they match your expected behavior? If not write comments in the test describing why they are not behaving as expected.

The test is updated such that the `@Error` handler returns a more
unusual status code (418) as a means of ensuring in the assertions that
the handler was definitely invoked.

A comment is added to the
`testErrorHandlerWithFluxChunkedSignaledImmediateError` test method
that explains the currently demonstrated behavior vs what would be
preferred in order to be less surprising in behavior.
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 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 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jeremyg484
Copy link
Contributor Author

jeremyg484 commented Oct 26, 2023

In separate discussion with @yawkat, it was confirmed that my assumptions in questions (1) and (3) are correct and the existing behavior is as intended. Re: question (2), he also confirmed that it might be possible, but difficult, to change the implementation such that the error handler still gets invoked in the chunked response case if the error is signaled immediately before any data.

I think it would be good to do this if we can, as it would be more consistent with the behavior of immediately thrown exceptions and less surprising to the user. @sdelamo I have added a comment to the testErrorHandlerWithFluxChunkedSignaledImmediateError method to point out this desired change. I will investigate the implementation further to see how it might be done.

The test has also been updated to have the error handler produce a more unusual status code (418) to better ensure the assertions are correctly identifying that the handler was invoked.

@sdelamo sdelamo merged commit 6271822 into 4.1.x Oct 27, 2023
10 checks passed
@sdelamo sdelamo deleted the flux-error-handlers branch October 27, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants