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(ext/node/ops/zlib/brotli): Allow decompressing more than 4096 bytes #20301

Merged

Conversation

lachrimae
Copy link
Contributor

@lachrimae lachrimae commented Aug 27, 2023

See issue #19816

In that issue, I suggest switching over the other brotli functionality to the Rust API provided by the brotli crate. Here, I only do that with the brotli_decompress function to fix the bug with buffers longer than 4096 bytes.

A good follow-up might be to repeat this change throughout the rest of the module. That will involve some actual work though, because the Node.js Brotli API is presumably based on the C google/brotli API, so an adapter layer in this module to map a Node.js Brotli API to the Rust API would end up being a little more complicated.

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2023

CLA assistant check
All committers have signed the CLA.

@lachrimae lachrimae marked this pull request as ready for review August 27, 2023 01:58
@lachrimae lachrimae changed the title fix(ext/node): Allow decompressing more than 4096 bytes fix(ext/node/ops/zlib/brotli): Allow decompressing more than 4096 bytes Aug 27, 2023
@lachrimae
Copy link
Contributor Author

lachrimae commented Aug 27, 2023

To project maintainers: I haven't been able to test this locally. There is some sort of issue where my test_util/std folder is empty, but tests expect it to have contents. Any advice on what's going on there? Is there a setup command I need to run?

EDIT: nevermind, I see that they are git submodules.

@littledivy
Copy link
Member

littledivy commented Aug 27, 2023

LGTM!

an adapter layer in this module to map a Node.js Brotli API to the Rust API would end up being a little more complicated.

Yes, thats why this module was written using the C API. We can use the Rust API for decompression but compression has a lot more flags https://nodejs.org/api/zlib.html#class-brotlioptions and no way to pass them using the Rust API.

EDIT: there are compression options https://docs.rs/brotli/latest/brotli/enc/backward_references/struct.BrotliEncoderParams.html

@lachrimae
Copy link
Contributor Author

lachrimae commented Aug 27, 2023

@littledivy okay cool, thank you for explaining those decisions! Would it be of value to the project if I created an adapter layer here, and in the result remove some unsafe code? Or is the Deno team fine with unsafe when used like this? I am a newcomer to the project, so I don't 100% know what the goals and values are 😃

@nberlette
Copy link
Contributor

What needs to happen for us to merge this branch? I'm working on a project right now that's at a complete standstill without a working brotli implementation in Deno 😣

@littledivy littledivy merged commit a9cc463 into denoland:main Sep 8, 2023
13 checks passed
@littledivy
Copy link
Member

Thanks for the ping @nberlette

bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Sep 8, 2023
…es (denoland#20301)

Fixes denoland#19816

In that issue, I suggest switching over the other brotli functionality
to the Rust API provided by the `brotli` crate. Here, I only do that
with the `brotli_decompress` function to fix the bug with buffers longer
than 4096 bytes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants