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

add lazyEntries option #22

Closed
thejoshwolfe opened this issue Dec 23, 2015 · 3 comments
Closed

add lazyEntries option #22

thejoshwolfe opened this issue Dec 23, 2015 · 3 comments
Labels

Comments

@thejoshwolfe
Copy link
Owner

One of the main goals of yauzl is to keep memory usage under control. The current pattern of entry and end events is hostile to this goal.

Using examples/unzip.js on http://wolfesoftware.com/crowded.zip.xz (unxz it first) results in over 2GB of virtual ram allocation as the program slows to a crawl while creating empty directories. This is failure. The problem is that we call openReadStream (0bf7f48#diff-5b07aa03e052f091324dde1dfbfd25bfR53) on every entry before pumping any of the file data. This is the naive solution to the problem that #18 ran into in the test system where autoClose can lead to race conditions.

Really, the problem is that autoClose expects you to call all your openReadStream()s before yauzl emits the end event, and yauzl races to emit the end event as soon as possible. Even with autoClose off, you need to keep all your Entry objects in ram as yauzl emits them, or you'll lose the information.

We need some way to throttle entry reading.

Also, it's questionable that yauzl's API encourages clients to extract all files in parallel. This would probably be worse performance on any file system that tries to pre-fetch data ahead of your read() calls (pretty much all of them, I think) than the normal strategy of extracting one file at a time from start to finish. I didn't test that, but it seems like in general file system access wouldn't necessarily benefit from parallelization; in fact, massive parallelization probably makes things much worse due to cache thrashing and the extra resources required to keep all the pipelines open.

It will be nice to remove examples/unzip.js's dependency on a third-party synchronization library pend, which is currently being used to iterate over the entries one at a time despite yauzl's API giving us all the entries in parallel. It's a smell that our own example doesn't use the recommended execution order.

@thejoshwolfe thejoshwolfe changed the title emitting entry events requires clients use RAM per entry add lazyEntries option Dec 23, 2015
@thejoshwolfe
Copy link
Owner Author

TODO: investigate alleged "undefined behavior" in the readme when calling close() before the end() event if we have lazyEntries: true.

@mathiasbynens
Copy link

Per https://github.com/thejoshwolfe/yauzl#openpath-options-callback, lazyEntries: false is still the default. Later in that section, it says “If lazyEntries is false […]. This is not recommended […]”.

If it’s not recommended, let’s change the default?

@thejoshwolfe
Copy link
Owner Author

thejoshwolfe commented May 6, 2018

let’s change the default

That would be an API breaking change, so it will happen at the major version bump for yauzl 3.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants