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

Decode percent encoding on-the-fly to avoid HttpData.usingBuffer #22

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

yawkat
Copy link
Contributor

@yawkat yawkat commented Jun 28, 2023

Motivation:

HttpData.usingBuffer may require copying data when the data is only present on disk. This is slow and defeats the purpose of moving the data to disk in the first place. Likely most large files will use multipart where this is not an issue, but the possibility still makes alternative HttpData implementations awkward. Thus, usingBuffer should be avoided where possible.

Modification:

Alter HttpPostStandardRequestDecoder to decode data on-the-fly instead of all at once.

Result:

usingBuffer is not called anymore. usingBuffer is now unused outside the HttpData implementations themselves.

Motivation:

HttpData.usingBuffer may require copying data when the data is only present on disk. This is slow and defeats the purpose of moving the data to disk in the first place. Likely most large files will use multipart where this is not an issue, but the possibility still makes alternative HttpData implementations awkward. Thus, usingBuffer should be avoided where possible.

Modification:

Alter HttpPostStandardRequestDecoder to decode data on-the-fly instead of all at once.

Result:

usingBuffer is not called anymore. usingBuffer is now unused outside the HttpData implementations themselves.
@yawkat
Copy link
Contributor Author

yawkat commented Jul 10, 2023

@pderop @violetagg can you take a look at this

@pderop
Copy link
Collaborator

pderop commented Jul 10, 2023

@yawkat ,

I will take a look (probably next wednesday), thanks.

@yawkat
Copy link
Contributor Author

yawkat commented Jul 10, 2023

thanks. no rush, i dont need this change urgently.

@yawkat
Copy link
Contributor Author

yawkat commented Aug 29, 2023

@pderop bump

@pderop pderop self-assigned this Aug 29, 2023
@pderop
Copy link
Collaborator

pderop commented Aug 30, 2023

@yawkat ,

can you please rebase your PR in order to pickup #23 ?

thanks !

@pderop
Copy link
Collaborator

pderop commented Aug 30, 2023

@yawkat ,

Before rebasing, maybe it's better to first wait for #24 to be merged, then at this point, please rebase, and then, the CI checks will then work again for your PR (well, ... hopefully :-))

thanks.

@pderop
Copy link
Collaborator

pderop commented Aug 31, 2023

@yawkat ,

thanks for this PR, I could review and validate it, nice improvement !

The reason why usingBuffer/usingContent has been introduced is explain in #17 (old getBuffer/content methods were also returning a copy in case of file-based HttpData).

Now, even in reactor-netty, we do not use usingBuffer/usingContent method, because unfortunately the contract is not yet clear, so maybe soon we will rework the usingContent/usingBuffer methods, I'm not yet sure.

The usingBuffer is still used here, but there is no copy in this case since the HttpData is memory based.
And it is also used in the tests in order to auto-close buffers when HttpData is file based.

@chrisvest or @normanmaurer , do you want to review this PR or may I merge it ? let me know

@yawkat
Copy link
Contributor Author

yawkat commented Aug 31, 2023

yea, in netty 4 micronaut would memory map the file for getBuffer so it still worked. but this is not possible anymore with netty 5. so this change became necessary for our implementation.

Copy link

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

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

LGTM!

@pderop
Copy link
Collaborator

pderop commented Sep 18, 2023

@franz1981 , thanks for the review !

@pderop pderop merged commit 0d5b0e4 into netty-contrib:main Sep 18, 2023
4 checks passed
@pderop pderop added this to the 5.0.0.Alpha2 milestone Sep 18, 2023
@pderop pderop added the enhancement New feature or request label Sep 18, 2023
@pderop
Copy link
Collaborator

pderop commented Sep 18, 2023

@yawkat , thanks a lot for the PR, which has just been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants