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.

Sign up for GitHub

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

Jump to bottom

'close' vs 'finish' events? #165

Open
d4hines opened this issue Jan 9, 2020 · 6 comments · May be fixed by #185
Open

'close' vs 'finish' events? #165

d4hines opened this issue Jan 9, 2020 · 6 comments · May be fixed by #185

Comments

@d4hines
Copy link

d4hines commented Jan 9, 2020

Hello,
Thank you for making this library - it's really helpful to us. I'm having trouble understanding the difference between the 'close' and 'finish' events - could you explain the difference?

The reason I ask is that we're hitting an intermittent error where sometimes not all of the folder is extracted, causing our program to fail later on. Looking at this post on SO, they listened to the 'finish' event, whereas we listen to the 'close' event, and I'm wondering if that might be connected.

Thanks!
@blackmad
Copy link

blackmad commented Mar 9, 2020

I'm having the opposite problem - I'm not getting a complete extraction of a file when I listen on 'finish,' but I do when I listen on 'close'

@blackmad
Copy link

blackmad commented Mar 9, 2020 •
edited
Loading

Here's a repro

this is the file:
https://www2.census.gov/geo/tiger/TIGER2016/ADDRFEAT/tl_2016_36005_addrfeat.zip

I wrote two little programs, one which extracts, waits on close and then exits, one which waits on finish - the extract in ~/Downloads is from using the built in mac unzip-on-doubleclick
https://gist.github.com/blackmad/a033c6ee245bfe9dba7683853f5731cf

  • node test-close.js
  • node test-finish.js
  • md5sum /Users/blackmad/Downloads/tl_2016_36005_addrfeat/tl_2016_36005_addrfeat.shx
    1fe6e3946b391144adf64dd63d138276 /Users/blackmad/Downloads/tl_2016_36005_addrfeat/tl_2016_36005_addrfeat.shx
  • md5sum ./tmp-close/tl_2016_36005_addrfeat.shx
    1fe6e3946b391144adf64dd63d138276 ./tmp-close/tl_2016_36005_addrfeat.shx
  • md5sum ./tmp-finish/tl_2016_36005_addrfeat.shx
    943e48cccea536eb59b6d2c73684b838 ./tmp-finish/tl_2016_36005_addrfeat.shx

the one that listens/exits on close appears to get this file (the last one in the archive) correct, the one that listens on finish doesn't

@ZJONSSON
Copy link
Owner

Thanks you for the report @d4hines and @blackmad really appreciate the report and the repro in particular, nicely done!!

Worth noting that I get the same checksum on all files on v12.13.0 on ubuntu so this might be platform specific error.

So the only difference between finish and close is one tick:

node-unzipper/lib/extract.js

Lines 43 to 45 in 9e38162

.on('finish',function() {
extract.emit('close');
});

But it appears that one tick is really needed. I think the fix might be to wrap the callbacks earlier within setImmediate to ensure all the i/o has truly flushed. This is still a little bit weird since the callbacks are only called when the writers emitted finished (might be problem with fstream)

@ZJONSSON ZJONSSON linked a pull request Mar 10, 2020 that will close this issue
Open
@ZJONSSON
Copy link
Owner

@blackmad can you run the repro using the extract-setimmediate branch?

npm install zjonsson/node-unzipper#extract-setimmediate

@blackmad
Copy link

blackmad commented Mar 10, 2020 via email

@blackmad
Copy link

blackmad commented Mar 10, 2020 via email

@karanbirsingh karanbirsingh mentioned this issue Apr 30, 2020
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Place cb behind setImmediate ZJONSSON/node-unzipper
3 participants
@blackmad @ZJONSSON @d4hines