Comments (9)
Thanks a lot for the assessment, it makes sense to me and I am looking forward to seeing it through.
The new release is in preparation and can happen once my PR was merged.
from flate2-rs.
You have a good point that parsing the header in the constructor is not idiomatic. But a change would be backwards-incompatible. If the inner reader is blocking then a caller can expect that, after a successful new
call, calling the header
method will always return a header.
from flate2-rs.
In this case calling header
would be the blocking call because that would initiate the process of issuing reads. So I believe construction could still be lazy, and you would not be breaking anything in a backwards incompatible way as I don't think users should be depending on construction being blocking vs calling header
or read
. I am imagining a solution similar to one I provided but probably less hacky. I think a following something like the lateinit pattern would work here?
from flate2-rs.
True. The header
method could read until the state is GzState::Body
and then return the header. Technically the same should be done for get_ref
, get_mut
and into_inner
so that they always return a reader that has read past the header. But we could get away with saying it was never specified in the docs that that was guaranteed.
from flate2-rs.
I feel whether get_ref
and friends should give a reader that is past the header is a decision that should be made, one way or the other, and then documented. If I had not known about the header skipping I would expect a stream that has been unmodified / not advanced, as those methods don't feel like they should have side effects given other Read
er types. For example making a BufReader
and then calling into_inner
does not pull any bytes from the underlying stream. The current new
and header
methods do not return a Result<_ io:error>
so its no entirely clear from the signature that there is work being done on the streams.
I feel it is probably ok to go ahead and make header
do the work of reading and buffer up the error until a read
is called as it does now.
from flate2-rs.
I'd be interested in understanding how the current behaviour of eagerly reading the header came to be.
What I don't particularly like about the API in its current state is that errors are delayed. From what I can tell, an error in the constructor will be exposed only when calling read()
.
Without any knowledge on how this came to be, I would tend to make this API more conventional, reporting errors when they occour and doing work only when needed, but reflect that, along with failure modes, in the signature of each method.
Making any change to signatures would probably break the symmetry among the different implementations, which makes it even more prohibitive and brings me back to trying to understand why this way of handling things was desirable in the first place, as I am clearly having a knowledge gap :/.
from flate2-rs.
Looking at some older code the reason for parsing the header seems to be that all the formats supported by flate2
are containers for DEFLATE encoded data and the different decoders exist to strip off the header and trailer of the specific format. The types get you to the point where read
decodes DEFLATE data and the header is mostly an inconvenience to be skipped.
In #185 support for async non-blocking was added. The first commit removes the header parsing from the constructor and the PR author makes the comment that "Usually in async code, we don't want to read IO before poll. The new behavior does not allow us to get gzip header before read." But then the 3rd commit adds it back with the message "keep old behavior".
from flate2-rs.
Thanks for the detective-work and the additional explanation. Regarding the header being an inconvenience to be skippedI could imagine that it was simply easiest to strip the header off the first chance one gets, disregarding that doing IO in the constructor might be unexpected or even problematic under circumstances.
To me it seems that there is no reason we'd know that would rate the current behaviour higher (to make it worth keeping) in the face of the problem its causing. To fix it, one would have to lazily parse the header during the first read
. The API wouldn't change which seems beneficial.
The behaviour of header()
isn't documented, so one might argue it's not a breaking change to adjust it and maybe document it while we are there. Existing code that relies on the header being present early would have to be adjusted to try the header after the first read or once the reading is done. To me, this seems like acceptable churn to fix an issue, which will leave not only this crate more robust, but also the calling code.
Trying to scrutinise the paragraph above, one might say previously one could use GzDecoder
to strip the header from a stream and read it, which wouldn't be possible anymore as the first read would set the stream past the header. Maybe that wouldn't be an issue with decoder.read(&[])
though, a subtle but explicit feature to restore the previous capability.
This looks like we should go ahead and make this type/these types lazy - what do you think?
from flate2-rs.
GzDecoder::read
already parses the header if it has not yet been parsed (since non-blocking reads may put it in that state). So the only change required is to replace the GzDecoder::new
header parsing with let state = GzState::Header(header_parser);
.
If the state is GzState::Header
then the header
method should call header_parser.parse
once, similar to current new
. This ensures that, for blocking reads, calling new
then header
on valid data still gives the header.
The as_ref
, as_mut
, and into_inner
methods should be documented as generally not guaranteeing how much data has been read from the inner type. For the read
module this is always true. For the bufread
module the guarantees are:
- when
read
returns 0 the next read will start immediately after the end of the compressed data - for blocking readers only, after calling
new
thenheader
the next read will start at the beginning of the DEFLATE data, matching the existing behaviour of just callingnew
.
Before making these technically backwards-incompatible changes (even if they are relied on by very few, if any, people) I suggest that there is a 1.0.27 release with the changes to main
, hopefully including #367
The changes suggested for this issue can then go into a 1.1.0 release so that if anyone does rely on the previous behaviour they can pin to 1.0 and we keep open the possibility of back-porting fixes to 1.0 if there is demand.
from flate2-rs.
Related Issues (20)
- Error on compiling flate2 on rust 1.57.0 HOT 2
- flate2::bufread::GzDecoder doesn't impl BufRead? HOT 3
- unsafe review: Potential (not actual) dangling pointers after inflate/deflate HOT 2
- total_in(&self) / total_out(&self) implementation for GzDecoder / GzEncoder / MultiGzDecoder HOT 2
- Issues with newly created file in read-write mode HOT 7
- Implement BufRead/Write for en/decoders alongside Read/Write
- rapidgzip
- Zlib succes while miniz_oxide fails HOT 5
- Testing validity of the data without the actual decompression
- Tree borrows violation occurs when using zlib backend HOT 5
- Some compressed files can only read a portion of the lines using GzDecoder. HOT 3
- question: Slowdown after upgrading from 1.0.26 to 1.0.28 HOT 8
- Using MultiGzDecoder for file with garbage after gzip data HOT 3
- Decoding a zip file returns the Error "corrupt deflate stream" HOT 2
- why GzDecoder can't read stream correct HOT 1
- Continue reading a stream after ZlibDecoder streams finishes HOT 8
- docs.rs failed to build flate2-1.0.29 HOT 1
- Add ability to set window_bits when using rust backend?
- unknown return code: -4 HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from flate2-rs.