Closed AlyoshaVasilieva closed 3 months ago
I'm also onboard with the general idea, but this
The window size is set for levels >=17, rather than >=20
seems rather arbitrary. I want to look at this more closely this or next week before it merges. Also looks like there's some CI problem.
edit: Nevermind, read the comment more closely now and it explains this.
Previous comment is now resolved, but there is another thing. The documentation for CParameter::window_log
says
Window size in bytes (as a power of two)
Since it says "in bytes", it seems like one would have to pass 8388608
(2 ^ 23), not 23
? Seems like it would be good to have a test, but maybe hard to do? I guess we should have one that shows that it works at all (i.e. you can make a request against a service using this middleware and decompress the body successfully).
Since it says "in bytes", it seems like one would have to pass
8388608
(2 ^ 23), not23
?
The docs are unclear, but it means that 23
is interpreted as 2 ^ 23. The C docs are slightly clearer:
"expressed as a power of 2".
Also the enum docs:
The actual distance is 2 power “this value”.
And the maximum possible setting is 31.
I think I can write a test for this, but I'm not sure I can do it without pulling axum
in as a dev dependency (been a while since I used any other HTTP server-type crate). The zstd
crate has a window_log_max
function on its decoder, so setting it to 23
and trying to decompress a response processed through a CompressionLayer set to maximum compression should work.
Alright, given the explanation I think we can merge as-is, though having a test before releasing this would be good.
cargo hack
is properly broken though, need to fix that (in a separate PR) before merging as I also don't have the permission to bypass the checks.
Added a test, which fails before the fix and passes after it.
Also bumped the zstd
dev dep to 0.13 just to keep up-to-date. It fixed 2 buffer overruns, though that doesn't really matter for test code.
When will a new version be released? Looking forward to this fix
zstd's memory buffer is limited to 8MB by Chromium and Firefox (at least), but the encoder can exceed that at level 20 and above. This ensures it's limited to 8MB.
See zstd issue 2712, RFC 8878, Chrome bug 41493659.
To verify, try this:
Cargo.toml:
main.rs:
Accessing
http://127.0.0.1:6612/zero
in recent Firefox/Chrome versions will trigger an error with tower-http 0.5.2, but works with this PR.Probably fixes #488. It should, but I haven't tested the software involved.
The window size is set for levels >=17, rather than >=20, in case
zstd
ever changes to have larger window sizes at lower presets for some reason, in the hopes that this will cover that too. (I don't think it's very likely that zstd will do that, but this shouldn't do anything at all for presets 17..=19, so it's not harmful.)