isaacs / node-tar

tar for node
ISC License
837 stars 183 forks source link

[BUG] Hangs on write method, writes but then the callback is not called. #360

Closed jcm10x1 closed 1 year ago

jcm10x1 commented 1 year ago

Is there an existing issue for this?

Current Behavior

I have an express server with TS that uses this package to extract a file from a tar upload. I use the write method of the stream to save the file. The file saves as it should, but then the callback is not called. Once Will write pubspec is logged, nothing else happens. Here is my code:

app.post("/api/packages/upload", authenticate, versioning, async (req: express.Request, res: express.Response) => {
    const files = req.files;
    if (!files) {
        return res.json({
            "error": {
                "code": "400",
                "message": "No files were uploaded."
            }
        })
    }
    Object.keys(files!).forEach(async (key) => {
        const uploadedFile: UploadedFile = files![key] as UploadedFile;
        await fs.mkdir("./tmp/pubspecs", {
            recursive: true
        });
        const tarStream = tar.extract({
            cwd: "./tmp/pubspecs"
        }, ["pubspec.yaml"]);
        const data = await fs.readFile(uploadedFile.tempFilePath);
        await new Promise<void>((resolve, reject) => {
            console.log("Will write pubspec");
            try {
                tarStream.write(data, (err) => {
                    if (err) {
                        console.log(err);
                        reject(err);
                    }
                    console.log("wrote pubspec");
                    resolve();
                });
            } catch (e) {
                console.log(e);
            }

        });
        console.log("Will read yaml");
        const pubspec = YAML.parse((await fs.readFile("./tmp/pubspecs/pubspec.yaml")).toString());
        console.log("read yaml");
        const name = pubspec.name;
        const version = pubspec.version;
        console.log("name: " + name);
        await uploadPackage(name, PackageType.DART, version, uploadedFile);
        res.status(204);
        res.location(`${process.env.URL_BASE}/dart/${name}/${version}/package.tar.gz`)
        await fs.rm(".tmp/pubspecs/pubspec.yaml");
        res.send();
    });
});

Here is my package.json:

{
  "name": "pub_server",
  "version": "0.0.1",
  "description": "This is a custom Dart Pub server for private packages.",
  "main": "build/index.js",
  "type": "module",
  "scripts": {
    "build": "tsc",
    "start": "tsc && node build/index.js",
    "dev": "tsc && nodemon build/index.js -w"
  },
  "dependencies": {
    "@google-cloud/storage": "^6.9.2",
    "@types/express": "^4.17.17",
    "@types/express-fileupload": "^1.4.1",
    "@types/tar": "^6.1.3",
    "@types/uuid": "^9.0.0",
    "dotenv": "^16.0.3",
    "express": "^4.17.2",
    "express-fileupload": "^1.4.0",
    "tar": "^4.4.19",
    "uuid": "^9.0.0",
    "yaml": "^2.2.1"
  },
  "devDependencies": {
    "typescript": "^4.0.0"
  }
}

Expected Behavior

I would expect the callback to be called and the code to continue once the file is written.

Steps To Reproduce

  1. TS with express in node 19.6.0
  2. See my package.json
  3. Run tsc && node
  4. Upload a tar archive to the endpoint
  5. See that the file saves but nothing else happens, the code gets hung.

Environment

isaacs commented 1 year ago

I'll take a look. In the meantime, does this work, instead of your tarStream.write() call?

tarStream.on('error', (err) => {
  console.log(err);
  reject(err);
});
tarStream.on('finish', () => {
  console.log("wrote pubspec");
  resolve();
});
tarStream.end(data);

At first glance, at least, it looks like you're leaking file descriptors, because the stream is never finished and closed.

isaacs commented 1 year ago

Also, you might try upgrading to v6, the version of tar you are using is quite a bit out of date.

jcm10x1 commented 1 year ago

Also, you might try upgrading to v6, the version of tar you are using is quite a bit out of date.

Yes, I downgraded to see if an earlier release did not have the bug. I will move back to v6. I will try your code above and get back to you. Thank you for the quick reply.

isaacs commented 1 year ago

Also, this bit here:

const tarStream = tar.extract({
            cwd: "./tmp/pubspecs"
        }, ["pubspec.yaml"]);
        const data = await fs.readFile(uploadedFile.tempFilePath);
        await new Promise<void>((resolve, reject) => {
            console.log("Will write pubspec");
            try {
                tarStream.write(data, (err) => {
                    if (err) {
                        console.log(err);
                        reject(err);
                    }
                    console.log("wrote pubspec");
                    resolve();
                });
            } catch (e) {
                console.log(e);
            }

        });

can probably be shortened to just:

        await tar.extract({
            cwd: "./tmp/pubspecs",
            file: uploadedFile.tempFilePath,
        }, ["pubspec.yaml"]);

since tar knows how to read files, and can then process it a bit more efficiently than reading the entire thing into memory and then pushing it through the stream yourself. (Really doesn't make much of a difference if the file is small, but it's more code that does the same thing.)