diff --git a/services/web/app/src/Features/Uploads/ArchiveManager.mjs b/services/web/app/src/Features/Uploads/ArchiveManager.mjs index 0982727484..995c394744 100644 --- a/services/web/app/src/Features/Uploads/ArchiveManager.mjs +++ b/services/web/app/src/Features/Uploads/ArchiveManager.mjs @@ -18,6 +18,7 @@ import OError from '@overleaf/o-error' import metrics from '@overleaf/metrics' import fs from 'node:fs' import Path from 'node:path' +import { pipeline } from 'node:stream' import yauzl from 'yauzl' import Settings from '@overleaf/settings' import { @@ -108,27 +109,16 @@ const ArchiveManager = { } callback = _.once(callback) - return zipfile.openReadStream(entry, function (err, readStream) { + fs.mkdir(Path.dirname(destFile), { recursive: true }, function (err) { if (err != null) { return callback(err) } - readStream.on('error', callback) - readStream.on('end', callback) - - const errorHandler = function (err) { - // clean up before calling callback - readStream.unpipe() - readStream.destroy() - return callback(err) - } - - fs.mkdir(Path.dirname(destFile), { recursive: true }, function (err) { + zipfile.openReadStream(entry, function (err, readStream) { if (err != null) { - return errorHandler(err) + return callback(err) } const writeStream = fs.createWriteStream(destFile) - writeStream.on('error', errorHandler) - return readStream.pipe(writeStream) + pipeline(readStream, writeStream, callback) }) }) }, diff --git a/services/web/test/unit/src/Uploads/ArchiveManager.test.mjs b/services/web/test/unit/src/Uploads/ArchiveManager.test.mjs index 4ab5e14b4f..67cc6ea798 100644 --- a/services/web/test/unit/src/Uploads/ArchiveManager.test.mjs +++ b/services/web/test/unit/src/Uploads/ArchiveManager.test.mjs @@ -10,6 +10,7 @@ import { vi, expect } from 'vitest' import sinon from 'sinon' import ArchiveErrors from '../../../../app/src/Features/Uploads/ArchiveErrors.mjs' import events from 'node:events' +import { PassThrough } from 'node:stream' vi.mock('../../../../app/src/Features/Uploads/ArchiveErrors.js', () => vi.importActual('../../../../app/src/Features/Uploads/ArchiveErrors.js') @@ -74,13 +75,13 @@ describe('ArchiveManager', function () { describe('successfully', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.readStream = new events.EventEmitter() - ctx.readStream.pipe = sinon.stub() + ctx.readStream = new PassThrough() ctx.zipfile.openReadStream = sinon .stub() .callsArgWith(1, null, ctx.readStream) - ctx.writeStream = new events.EventEmitter() + ctx.writeStream = new PassThrough() ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream) + sinon.spy(ctx.writeStream, 'destroy') ctx.ArchiveManager.extractZipArchive( ctx.source, ctx.destination, @@ -89,7 +90,7 @@ describe('ArchiveManager', function () { // entry contains a single file ctx.zipfile.emit('entry', { fileName: 'testfile.txt' }) - ctx.readStream.emit('end') + ctx.readStream.end() ctx.zipfile.emit('end') }) }) @@ -106,13 +107,13 @@ describe('ArchiveManager', function () { describe('with a zipfile containing an empty directory', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.readStream = new events.EventEmitter() - ctx.readStream.pipe = sinon.stub() + ctx.readStream = new PassThrough() ctx.zipfile.openReadStream = sinon .stub() .callsArgWith(1, null, ctx.readStream) - ctx.writeStream = new events.EventEmitter() + ctx.writeStream = new PassThrough() ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream) + sinon.spy(ctx.writeStream, 'destroy') ctx.ArchiveManager.extractZipArchive( ctx.source, ctx.destination, @@ -124,7 +125,7 @@ describe('ArchiveManager', function () { // entry contains a single, empty directory ctx.zipfile.emit('entry', { fileName: 'testdir/' }) - ctx.readStream.emit('end') + ctx.readStream.end() ctx.zipfile.emit('end') }) }) @@ -285,10 +286,10 @@ describe('ArchiveManager', function () { describe('with backslashes in the path', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.readStream = new events.EventEmitter() - ctx.readStream.pipe = sinon.stub() - ctx.writeStream = new events.EventEmitter() + ctx.readStream = new PassThrough() + ctx.writeStream = new PassThrough() ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream) + sinon.spy(ctx.writeStream, 'destroy') ctx.zipfile.openReadStream = sinon .stub() .callsArgWith(1, null, ctx.readStream) @@ -301,7 +302,9 @@ describe('ArchiveManager', function () { } ) ctx.zipfile.emit('entry', { fileName: 'wombat\\foo.tex' }) + ctx.readStream.end() ctx.zipfile.emit('entry', { fileName: 'potato\\bar.tex' }) + ctx.readStream.end() return ctx.zipfile.emit('end') }) }) @@ -362,7 +365,7 @@ describe('ArchiveManager', function () { ctx.zipfile.openReadStream = sinon .stub() .callsArgWith(1, new Error('Something went wrong')) - ctx.writeStream = new events.EventEmitter() + ctx.writeStream = new PassThrough() ctx.ArchiveManager.extractZipArchive( ctx.source, ctx.destination, @@ -392,13 +395,13 @@ describe('ArchiveManager', function () { describe('with an error in the file read stream', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.readStream = new events.EventEmitter() - ctx.readStream.pipe = sinon.stub() + ctx.readStream = new PassThrough() ctx.zipfile.openReadStream = sinon .stub() .callsArgWith(1, null, ctx.readStream) - ctx.writeStream = new events.EventEmitter() + ctx.writeStream = new PassThrough() ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream) + sinon.spy(ctx.writeStream, 'destroy') ctx.ArchiveManager.extractZipArchive( ctx.source, ctx.destination, @@ -409,7 +412,6 @@ describe('ArchiveManager', function () { ) ctx.zipfile.emit('entry', { fileName: 'testfile.txt' }) ctx.readStream.emit('error', new Error('Something went wrong')) - ctx.zipfile.emit('end') }) }) @@ -429,15 +431,14 @@ describe('ArchiveManager', function () { describe('with an error in the file write stream', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.readStream = new events.EventEmitter() - ctx.readStream.pipe = sinon.stub() - ctx.readStream.unpipe = sinon.stub() - ctx.readStream.destroy = sinon.stub() + ctx.readStream = new PassThrough() + sinon.spy(ctx.readStream, 'destroy') ctx.zipfile.openReadStream = sinon .stub() .callsArgWith(1, null, ctx.readStream) - ctx.writeStream = new events.EventEmitter() + ctx.writeStream = new PassThrough() ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream) + sinon.spy(ctx.writeStream, 'destroy') ctx.ArchiveManager.extractZipArchive( ctx.source, ctx.destination, @@ -448,7 +449,6 @@ describe('ArchiveManager', function () { ) ctx.zipfile.emit('entry', { fileName: 'testfile.txt' }) ctx.writeStream.emit('error', new Error('Something went wrong')) - ctx.zipfile.emit('end') }) }) @@ -459,11 +459,6 @@ describe('ArchiveManager', function () { .and(sinon.match.has('message', 'Something went wrong')) ) }) - - it('should unpipe from the readstream', function (ctx) { - ctx.readStream.unpipe.called.should.equal(true) - }) - it('should destroy the readstream', function (ctx) { ctx.readStream.destroy.called.should.equal(true) })