Add safe Rust zstd decoding backend#349
Conversation
197g
left a comment
There was a problem hiding this comment.
I'd gladly add it optionally, similar to rustls being an option for SSL crates. @KillingSpark If you have the time to chime in and maybe some words on the integration?
| #[cfg(all(not(feature = "zstd"), feature = "safe-rust-zstd"))] | ||
| CompressionMethod::ZSTD => Box::new( | ||
| ruzstd::decoding::StreamingDecoder::new(reader) | ||
| .map_err(|e| std::io::Error::other(e))?, | ||
| ), |
There was a problem hiding this comment.
I find it a little odd to get an error already from construction that is related to the contents rather than the parameters. Well, a bit hypocritical, considering I'm currently fixing us having the same mistake in Decoder::new.
There was a problem hiding this comment.
They return errors in the new method for the same reason we currently do: that's where header parsing happens
|
Another naming option would be |
|
There is a documented caveat in the ruzstd streaming decoder: it only decodes a single frame from the stream, while Zstd streams can contain multiple frames. But multi-frame files are relatively uncommon and the fix should be committed upstream into |
|
Hi everyone, the code in the PR looks good. A few things:
|
Pros: 100% safe Rust. No memory safety bugs, no C toolchain required
Cons: ~3x slower than libzstd
Future compatibility notes
The safe Rust impl has no encoding support. That's fine for now since the crate doesn't support encoding Zstd anywyay. When encoding is implemented, we'll have to limit it to the zstd feature only, which should be trivial.
Trifecta Tech Foundation is starting work on a Rust drop-in replacement for libzstd, similar to zlib-rs. When that's ready to use, I expect it to replace the
zstdfeature, and this option to remain available.