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

feat: Extract All Async, Extract File Async #209

Closed
wants to merge 2 commits into from
Closed

feat: Extract All Async, Extract File Async #209

wants to merge 2 commits into from

Conversation

waseemshahwan
Copy link

New to open source contributions. All criticism is welcome.

I've had this problem myself when using with an electron process; the window shows as non-responding when extracting which I would have liked to fix. I also found that someone had this problem in issue #167 and it's been open for a while.

@waseemshahwan waseemshahwan changed the title Extract All Async, Extract File Async feat: Extract All Async, Extract File Async Nov 29, 2020
@malept
Copy link
Member

malept commented Dec 3, 2020

How does this solution address #167 (comment) ?

@waseemshahwan
Copy link
Author

How does this solution address #167 (comment) ?

Not sure what is meant in that comment. I just substituted all filesystem operation functions to use their async counterparts (fs.readFile vs fs.readFileSync). This does not block the event loop and therefore can be wrapped in a promise and run asynchronously.

After some tests, this method worked. The process was no longer being blocked by these operations and the output was as expected.

@malept
Copy link
Member

malept commented Dec 4, 2020

I believe it means that there's an inherent flaw in the file format where in certain cases, it's not safe to be asynchronously read the archive, especially with extractAll.

@waseemshahwan
Copy link
Author

I believe it means that there's an inherent flaw in the file format where in certain cases, it's not safe to be asynchronously read the archive, especially with extractAll.

Unless there is a fundamental difference between the native fs sync and non-sync functions, I can't imagine that to be the case. If the original commentator @MarshallOfSound can produce this potential flaw, I can look into possible fixes.

The fs.read and fs.readSync should always have the same behavior, and if they don't, then that is a flaw in the native package itself.

@ibrahimelement
Copy link

Waiting for this to be approved, desperately needed right now... Our team has been tracking down a bug related to this issue... Thanks waseem!

@Nantris
Copy link

Nantris commented Mar 3, 2021

Any update on this? Looks promising.

@Nantris
Copy link

Nantris commented Dec 14, 2021

@waseemshahwan why closed? I think this would be a great addition!

@waseemshahwan
Copy link
Author

If authors of the repo are interested in merging I can reopen.

@Nantris
Copy link

Nantris commented Dec 14, 2021

@zcbenz @MarshallOfSound

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

Successfully merging this pull request may close these issues.

4 participants