EvanOxfeld / node-unzip

node.js cross-platform unzip using streams
MIT License
613 stars 343 forks source link

Extract doesn't emit "close" event #80

Open vucuong12 opened 9 years ago

vucuong12 commented 9 years ago

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 :)

mariohmol commented 6 years ago

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

ZJONSSON commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

mariohmol commented 6 years ago

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

mariohmol commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Thanks for the clarification.

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