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

Question: should `.finalize` be awaited?

Open nicojs opened this issue 1 year ago • 6 comments

After upgrading eslint, I noticed that .finalize() returns a promise. I can see that this is also documented here: https://www.archiverjs.com/docs/archiver#finalize. However, it has yet to be awaited in the readme example. Can I simply await this promise instead of using the error and close event handlers?

To be clear:

This:

async function zip(from: string, to: string) {
	const output = fs.createWriteStream(to);
	const archive = archiver('zip', {
		zlib: { level: 9 }, // Sets the compression level.
	});
	archive.pipe(output);
	archive.directory(from, false);
	await archive.finalize();
	console.log(`✅ ${to} (${archive.pointer()} bytes)`);
}

Instead of this:

function zip(from: string, to: string) {
	return new Promise<void>((res, rej) => {
		const output = fs.createWriteStream(to);
		const archive = archiver('zip', {
			zlib: { level: 9 }, // Sets the compression level.
		});
		output.on('close', () => {
			console.log(`✅ ${to} (${archive.pointer()} bytes)`);
			res();
		});
		archive.on('error', err => rej(err));
		archive.pipe(output);
		archive.directory(from, false);
		archive.finalize();
      }
}

nicojs avatar Jul 04 '24 17:07 nicojs

same

Rabbitzzc avatar Jul 24 '24 01:07 Rabbitzzc

My answer to this - after spending probably 20 hours trying to resolve an issue whereby my zips where either never finalized or corrupted, is NO ! Certainly not when you are piping to a stream. If you await the finalize then you are only going to get lucky if its called and archiver has already finished processing the files you gave it - i.e. a small number of small files. If you are zipping up anything more than a few KB you will almost certainly fall into the same hole I did, whereby the await never resolves. Just set up your stream, pipe the archiver instance to it, feed it your files, and - in my case anyway - set the stream as the HTTP response. It will sort itself out nicely and just work as you'd hope . . .

andrewlorenz avatar Sep 26 '24 16:09 andrewlorenz

~But if the destination is a file? In that case, one should await, right?~ It is still a stream, I suppose. But what should one do with the returned promise?

IvanUkhov avatar Oct 28 '24 13:10 IvanUkhov

No you shouldn't await finalize. It seems to fail silently if you await finalize() and your code will exit without showing any errors. Very odd. I've tested this on a barebones js function in an AWS Lambda function. You should wrap your function in a promise and resolve when the "finish" event is fired from the archiver OR if you're passing in an uploadStream (to s3 for example), wait for the uploadStream to fire "close" or "finish".

jeff2013 avatar Dec 12 '24 23:12 jeff2013

The issue with await archive.finalize() appears to come from a circular dependency in the archiver's internal design. The finalize() promise only resolves after the internal module emits its 'end' event, but this event depends on the output stream completing, which in turn requires the consumer to read the data. However, in many implementations (especially with HTTP responses or file streams), the consumer can't start reading until the function returns - which is blocked waiting for finalize().

If finalize() should not be awaited it should not return Promise. But actually there are reasons...

ciekawy avatar Jan 23 '25 14:01 ciekawy

poop api

stevenvachon avatar Mar 19 '25 19:03 stevenvachon