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

Stop double encoding body when we have a response encoder #550

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

timyates
Copy link
Contributor

When a response-encoder is present, it handles encoding the response.

Without this change, we were then falling through and encoding the body into the response a second time.

This was discovered in AWS here micronaut-projects/micronaut-aws#1843

It wasn't seen here as we add a servlet for handling static resources in jetty/undertow/tomcat

When a response-encoder is present, it handles encoding the response.

Without this change, we were then falling through and encoding the body into the response a second time.

This was discovered in AWS here micronaut-projects/micronaut-aws#1843

It wasn't seen here as we add a servlet for handling static resources in jetty/undertow/tomcat
@dstepanov
Copy link
Contributor

Can this be tested?

@timyates
Copy link
Contributor Author

@dstepanov Trying to think of a way to test it, but struggling atm... 🤔

@timyates
Copy link
Contributor Author

@dstepanov As far as I can tell, tomcat/jetty/undertow take the async path. It's only when we get to AWS or GCP that the synch path is taken and this bug is exhibited 🤔

@dstepanov
Copy link
Contributor

Maybe we can force the servlet engine to always use sync

@timyates
Copy link
Contributor Author

@dstepanov Can do something like this:

6dd7cb9

So use a "secret" property to switch to sync mode (but default to async mode)

Or should this be a public config?

@sonarqubecloud
Copy link

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@dstepanov
Copy link
Contributor

The secret property is fine with me. I thought a servlet engine like Jetty would have an internal option to disable async, but it might be part of the spec, so probably not.

@sdelamo
Copy link
Contributor

sdelamo commented Aug 22, 2023

ping @dstepanov

@sdelamo sdelamo merged commit ed8ac1f into 4.0.x Aug 22, 2023
@sdelamo sdelamo deleted the response-encoder-fix branch August 22, 2023 15:21
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