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

Different behavior with yauzl #352

Open
regseb opened this issue Mar 3, 2022 · 7 comments · May be fixed by #360
Open

Different behavior with yauzl #352

regseb opened this issue Mar 3, 2022 · 7 comments · May be fixed by #360

Comments

@regseb
Copy link

regseb commented Mar 3, 2022

The yauzl library has a different behavior with mock-fs.

import mock from "mock-fs";
import yauzl from "yauzl";

if (process.argv.includes("mock")) {
    mock({ "baz.zip": mock.load("baz.zip") });
}

const zipfile = await new Promise((resolve, reject) => {
    yauzl.open("baz.zip", (err, zipfile) => {
        if (err) reject(err);
        resolve(zipfile);
    });
});
zipfile.on("error", (err) => console.log("error", err));
zipfile.on("entry", (entry) => console.log("entry", entry.fileName));
zipfile.on("end", () => console.log("end"));

The above code prints :

  • without the mock argument: entry foo.txt and end
  • with the mock argument : end

I think the problem is that mock-fs does not call IO and returns the result directly (Event Loop). Because without the promise, the behavior is the same.

yauzl.open("baz.zip", (err, zipfile) => {
    if (err) throw err;
    zipfile.on("error", (err) => console.log("error", err));
    zipfile.on("entry", (entry) => console.log("entry", entry.fileName));
    zipfile.on("end", () => console.log("end"));
});

To reproduce the problem:

  • Download and unzip testcase.zip
  • npm install
  • node fail.js
  • node fail.js mock
@regseb
Copy link
Author

regseb commented Mar 31, 2022

@3cp

  • node: v16.14.2
  • npm: 8.5.5
  • Ubuntu: 20.04.4 LTS

The scripts have a mock parameter to activate mock-fs. node fail.js launches the reading of the zip without anything mocked. node fail.js mock launches the reading of the zip with mock-fs. success.js works with and without mock-fs. But fail.js doesn't work with mock-fs.

node fail.js node fail.js mock node success.js node success.js mock
entry foo.txt
end
end
entry foo.txt
end
entry foo.txt
end

@3cp
Copy link
Collaborator

3cp commented Mar 31, 2022

My bad, I missed the mock argument. I can reproduce the issue on both node v14 and v16.

@3cp
Copy link
Collaborator

3cp commented Mar 31, 2022

I don't quite understand this Nodejs (or JavaScript) feature, the promise that handles events "error", "entry" and "end". I thought only the reject and resolve calls interact with the promise itself?
Is there some official document around the wrapped promise becoming an event target?

@regseb
Copy link
Author

regseb commented Apr 1, 2022

The variable globalZipfile contains the value of the variable callbackZipfile. The await waits the promise and returns the fulfilled value of the promise (or throws an error if the promise is rejected).

const globalZipfile = await new Promise((resolve, reject) => {
    yauzl.open("baz.zip", (err, callbackZipfile) => {
        if (err) reject(err);
        resolve(callbackZipfile);
    });
});

@regseb
Copy link
Author

regseb commented Apr 1, 2022

Here is another test case using setImmediate() that fails with mock-fs.

import mock from "mock-fs";
import yauzl from "yauzl";

if (process.argv.includes("mock")) {
    mock({ "baz.zip": mock.load("baz.zip") });
}

yauzl.open("baz.zip", (err, zipfile) => {
    if (err) throw err;
    setImmediate(() => {
        zipfile.on("error", (err) => console.log("error", err));
        zipfile.on("entry", (entry) => console.log("entry", entry.fileName));
        zipfile.on("end", () => console.log("end"));
    });
});

@3cp
Copy link
Collaborator

3cp commented Apr 1, 2022

Oh, I see. I got confused. It's the resolved value in play.

@3cp
Copy link
Collaborator

3cp commented Apr 1, 2022

Feels like another timing issue because of our readStream patch.
The zipfile already emitted those entry events before the promise exercise the .then(resolved => {...}).
The lazyEntries mode of yauzl seems work as expected.

@regseb regseb linked a pull request Aug 1, 2022 that will close this issue
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 a pull request may close this issue.

2 participants