node-unzip icon indicating copy to clipboard operation
node-unzip copied to clipboard

Extract doesn't emit "close" event

Open vucuong12 opened this issue 10 years ago • 10 comments

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 avatar Aug 18 '15 07:08 vucuong12

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

mariohmol avatar Jul 03 '18 18:07 mariohmol

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(...)

ZJONSSON avatar Jul 03 '18 21:07 ZJONSSON

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

mariohmol avatar Jul 03 '18 22:07 mariohmol

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 ([email protected]). Also can you tell me which version of unzipper you are using (if not the latest one).

As @rhodgkins has pointed out in https://github.com/ZJONSSON/node-unzipper/issues/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.

ZJONSSON avatar Jul 03 '18 23:07 ZJONSSON

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

mariohmol avatar Jul 03 '18 23:07 mariohmol

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);
        });
      })

mariohmol avatar Jul 04 '18 00:07 mariohmol

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 [email protected] 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);
      });
    });
  }); 

ZJONSSON avatar Jul 04 '18 00:07 ZJONSSON

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()}/`;

mariohmol avatar Jul 04 '18 01:07 mariohmol

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)

ZJONSSON avatar Jul 04 '18 01:07 ZJONSSON

Thanks for the clarification.

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

mariohmol avatar Jul 04 '18 12:07 mariohmol