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

Document why Decoder::new might return Err #281

Open
edmorley opened this issue Apr 23, 2024 · 1 comment
Open

Document why Decoder::new might return Err #281

edmorley opened this issue Apr 23, 2024 · 1 comment

Comments

@edmorley
Copy link

edmorley commented Apr 23, 2024

Migrating from flate2 to the zstd crate, it was surprising to me to find that zstd::stream::read::Decoder::new returns an io::Result:

impl<R: Read> Decoder<'static, BufReader<R>> {
/// Creates a new decoder.
pub fn new(reader: R) -> io::Result<Self> {
let buffer_size = zstd_safe::DCtx::in_size();
Self::with_buffer(BufReader::with_capacity(buffer_size, reader))
}
}

Compare this to flate2::read::GzDecoder::new, which returns the decoder directly:
https://docs.rs/flate2/latest/flate2/read/struct.GzDecoder.html#method.new

I was hoping the rustdocs for zstd::stream::read::Decoder::new might explain why it is fallible, so I could determine whether specific user facing error handling was required for this failure mode, or whether it was unlikely to occur in practice.

From inspection, it seems the only two fallible calls during the creation of the decoder are here:

zstd-rs/src/stream/raw.rs

Lines 147 to 150 in e470f00

context.init().map_err(map_error_code)?;
context
.load_dictionary(dictionary)
.map_err(map_error_code)?;

...which are:

  1. Initialising the decompression context using zstd_sys::ZSTD_initDStream
  2. Loading the dictionary using zstd_sys::ZSTD_DCtx_loadDictionary

Checking the zstd source for ZSTD_initDStream I see:
https://github.com/facebook/zstd/blob/ff7a151f2e6c009b657d9f798c2d9962b0e3feb5/lib/decompress/zstd_decompress.c#L1747-L1754

...where the code comment suggests it can't fail?

Similarly, checking ZSTD_DCtx_loadDictionary I see the fallible paths appear to only be encountered if trying to set the dictionary when the context is in the wrong state (which can't happen unless zstd-safe has a bug), or when a non-empty dictionary is provided (and Decoder::new sets an empty dictionary):
https://github.com/facebook/zstd/blob/ff7a151f2e6c009b657d9f798c2d9962b0e3feb5/lib/decompress/zstd_decompress.c#L1697-L1711

...so unless I'm missing something it appears Decoder::new() can never fail in practice?

@gyscos
Copy link
Owner

gyscos commented Jul 9, 2024

Hi, and thanks for the report!

Indeed, it looks like this should never return an error in practice.

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

No branches or pull requests

2 participants