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

Extract doesn't emit "close" event #80

Open
vucuong12 opened this issue Aug 18, 2015 · 10 comments
Open

Extract doesn't emit "close" event #80

vucuong12 opened this issue Aug 18, 2015 · 10 comments

Comments

@vucuong12
Copy link

I cannot unzip the file I download from this site. http://subscene.com/subtitles/the-moth-diaries/english/627840
The problem is: unzip.Extract never emit "close" event, and the entry (the .srt file) has 0 KB (as in the picture)

pic1

This is a really rare case. Hope you can fix it :)

@vucuong12 vucuong12 reopened this Aug 18, 2015
@vucuong12 vucuong12 changed the title Extract doesn Extract doesn't emit "close" event Aug 18, 2015
@mariohmol
Copy link

Same here.. close is never fired event when finished the extract

@ZJONSSON
Copy link

ZJONSSON commented Jul 3, 2018

You can try unzipper, a drop in replacement for node-unzip that is actively managed.

In the example above, here are three different ways to extract the file successfully (first two are compatible with node-unzip the other 2 are new)

const unzipper = require('./node-unzipper');
const fs = require('fs');
const path = require('path');
const filePath = '/tmp/test-subtitles/the-moth-diaries_english-627840.zip';
const outputDir = '/tmp/test-subtitles';  // this directory has to exist already

// Extract
fs.createReadStream(filePath)
  .pipe(unzipper.Extract({path: outputDir}))
  .on('close', () => console.log('Extract closed'));

// Parse each file with .on('entry',...) 
fs.createReadStream(filePath)
  .pipe(unzipper.Parse())
  .on('entry', entry => {
    entry.pipe(fs.createWriteStream(path.join(outputDir, 'parse.srt')))
      .on('finish', () => console.log('parse done'));
  });

// ParseOne (parses the first file)
fs.createReadStream(filePath)
  .pipe(unzipper.ParseOne())
  .pipe(fs.createWriteStream(path.join(outputDir,'parseOne.srt')))
  .on('finish', () => console.log('parseOne done'));

// Open.file (opens the central directory and parses the first file)
unzipper.Open.file(filePath)
  .then(directory => {
    directory.files[0].stream()
      .pipe(fs.createWriteStream(path.join(outputDir,'open.srt')))
      .on('finish', () => console.log('open done'));
  });

You can also unzip streaming from a url by using Open.url(...)

@mariohmol
Copy link

mariohmol commented Jul 3, 2018

Hi.. tryed using unzipper, close its triggered but the folder its empty in that time:


    fs.createReadStream(filepath).pipe(unzipper.Extract({ path: destFolder }))
      .on('close', () => {
        console.log(destFolder);
        res.json({});
        fs.readdir(destFolder, (err, files) => {
          console.log(err, files, `<----`)
          files.forEach(fileFound => {
            console.log(fileFound);
          });
        });
      }); 

console:
null [] '<----'

Actually this is not making the unzip.. the destFolder still empty.. with node-unzip was unzipping

@ZJONSSON
Copy link

ZJONSSON commented Jul 3, 2018

Thanks for the report @mariohmol. Is the zip file you are looking at public or sharable - if so can you share a link? If you don't want it public you can also email me the file (ziggy.jonsson.nyc@gmail.com). Also can you tell me which version of unzipper you are using (if not the latest one).

As @rhodgkins has pointed out in ZJONSSON#57 we emit close and finish even when there is an error, which can be misleading (we have to fix). So you might try to attach an error handler, i.e. just add .on('error', console.log) and see if anything is uncaught.

@mariohmol
Copy link

No errors with error event, using version "unzipper": "^0.9.1" and its a normal zip using mac
Arquivo Comprimido 2.zip

@mariohmol
Copy link

That worked:

import * as extract from 'extract-zip';
    extract(filepath, { dir: destFolder }, function (error) {
      // extraction is complete. make sure to handle the err
      console.log(error);
      res.json({});
      fs.readdir(destFolder, (err, files) => {
        console.log(err, files, `<----`);
        files.forEach(fileFound => {
          console.log(fileFound);
        });
      })

@ZJONSSON
Copy link

ZJONSSON commented Jul 4, 2018

Thanks @mariohmol, awesome that you got this working with extract-zip! It's based on yazul which is a very good unzip module but with slightly different functionality than unzipper and not a drop-in replacement for node-unzip

I ran your exact example with unzipper@0.9.1 with both node 9.11.2 and node 10.5.0 and got the following output without any failure.

/tmp/test-arquivo
null [ 'Image from iOS (5).png',
  'Image from iOS (9).png',
  '__MACOSX' ] '<----'
Image from iOS (5).png
Image from iOS (9).png
__MACOSX

Would you be so kind to verify the node version you are running so I can make sure I am not missing anything?

If you want to debug this further please note that errors do not propagate. So if you put an error handler on every step of the way you might be able to catch what went wrong, i.e.:

fs.createReadStream(filepath)
  .on('error', console.log)
  .pipe(unzipper.Extract({ path: destFolder }))
  .on('error', console.log)
  .on('close', () => {
    console.log(destFolder);
    res.json({});
    fs.readdir(destFolder, (err, files) => {
      console.log(err, files, `<----`)
      files.forEach(fileFound => {
        console.log(fileFound);
      });
    });
  }); 

@mariohmol
Copy link

Thanks for your attention!

my node: v6.11.2

I found the issue, the filepath must be relative but the folderPath must be absolute.

I could reproduce the error:

This gives the error:

    const mainUploadRelative =  `${__dirname}/../../../upload/temp`;
    const mainUpload = path.resolve(mainUploadRelative);
    const filepath = mainUploadRelative + `/${file.filename}`;
    const destFolder = mainUploadRelative + `/${new Date().getTime()}/`;

When i change the last line , it works

const destFolder = mainUpload + `/${new Date().getTime()}/`;

@ZJONSSON
Copy link

ZJONSSON commented Jul 4, 2018

Excellent, thank you @mariohmol for investigating further. Great to get to the bottom of this!
Unfortunately the native-streams error mechanism is missing error propagation.

Ideally we should be able to catch all errors and the final response of a stream pipeline by something like .promise (where next is the express error handler):

  .pipe(....)
  .pipe(....)
  .promise()
  .then(data => res.json(data), next)

..,but we aren't there yet in native streams (here is my custom implementation with propagation tests)

@mariohmol
Copy link

Thanks for the clarification.

Isnt possible to test and check if works relative and absolute? so the dest folder will work anyway?

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

3 participants