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

Fix Error handler rebinding the body #548

Merged
merged 2 commits into from
Aug 22, 2023
Merged

Fix Error handler rebinding the body #548

merged 2 commits into from
Aug 22, 2023

Conversation

timyates
Copy link
Contributor

When we read the body for a request, and then try to re-read it for a different Body type in an error handler, we fail as the stream is already exhausted

This PR caches the input-stream and re-sets it whenever the input-stream is re-required

Also updated micronaut core, reactor and undertow

When we read the body for a request, and then try to re-read it for a different Body type in
an error handler, we fail as the stream is already exhausted

This PR caches the input-stream and re-sets it whenever the input-stream is re-required

Also updated micronaut core, reactor and undertow
@timyates timyates added the type: bug Something isn't working label Aug 18, 2023
@timyates timyates requested review from sdelamo and yawkat August 18, 2023 13:16
@timyates timyates self-assigned this Aug 18, 2023
timyates added a commit to micronaut-projects/micronaut-aws that referenced this pull request Aug 18, 2023
@yawkat
Copy link
Member

yawkat commented Aug 21, 2023

this change has severe performance implications. is it necessary? netty does not do this.

@sdelamo
Copy link
Contributor

sdelamo commented Aug 21, 2023

At the minimum, we must document that rebinding to a different type does not work.

sdelamo added a commit to micronaut-projects/micronaut-aws that referenced this pull request Aug 21, 2023
@timyates
Copy link
Contributor Author

@yawkat The ServletBodyBinder reads the request body from getInputStream.

This fails as the body is invalid json

But then the error handler tries to read the body as a String, so it goes back through the servletbodybinder and fails.

I'm not sure how netty does it, I assume it's working on buffers instead of inputstreams...

@sdelamo I am happy with this being a simple documentation change instead of this caching of all request bodys

@yawkat
Copy link
Member

yawkat commented Aug 21, 2023

netty only buffers for json. imo modifying getInputStream to always buffer is not worth it. there are plenty of (non-json) streaming use cases where buffering may be prohibitive.

@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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@timyates
Copy link
Contributor Author

timyates commented Aug 21, 2023

Ok, added docs around known issues with servlet and ignored the test in the TCKs

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