From 6bf88415609a1c5a9132ba42fe575205542f5d6f Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 2 Aug 2023 15:01:29 +0200 Subject: [PATCH] Merge pull request #14103 from overleaf/jpa-web-create-dump-once [web] create the dump folder once at startup GitOrigin-RevId: 0026ebe15a92f0d17f97966c89cb471b1282d061 --- services/web/app.js | 5 + .../app/src/Features/LinkedFiles/UrlAgent.js | 1 - .../Features/Templates/TemplatesManager.js | 140 +++++++++--------- .../web/app/src/infrastructure/FileWriter.js | 109 ++++++-------- .../src/Templates/TemplatesManagerTests.js | 6 - 5 files changed, 115 insertions(+), 146 deletions(-) diff --git a/services/web/app.js b/services/web/app.js index 148f9df730..80f939b904 100644 --- a/services/web/app.js +++ b/services/web/app.js @@ -44,6 +44,7 @@ const mongoose = require('./app/src/infrastructure/Mongoose') const { triggerGracefulShutdown, } = require('./app/src/infrastructure/GracefulShutdown') +const FileWriter = require('./app/src/infrastructure/FileWriter') if (Settings.catchErrors) { process.removeAllListeners('uncaughtException') @@ -51,6 +52,10 @@ if (Settings.catchErrors) { logger.error({ err: error }, 'uncaughtException') ) } + +// Create ./data/dumpFolder if needed +FileWriter.ensureDumpFolderExists() + const port = Settings.port || Settings.internal.web.port || 3000 const host = Settings.internal.web.host || 'localhost' if (!module.parent) { diff --git a/services/web/app/src/Features/LinkedFiles/UrlAgent.js b/services/web/app/src/Features/LinkedFiles/UrlAgent.js index e2b0e6cb21..325896c116 100644 --- a/services/web/app/src/Features/LinkedFiles/UrlAgent.js +++ b/services/web/app/src/Features/LinkedFiles/UrlAgent.js @@ -84,7 +84,6 @@ function _getUrlStream(projectId, data, currentUserId, callback) { } url = UrlHelper.wrapUrlWithProxy(url) const readStream = request.get(url) - readStream.pause() callback(null, readStream) } diff --git a/services/web/app/src/Features/Templates/TemplatesManager.js b/services/web/app/src/Features/Templates/TemplatesManager.js index 72d8305075..3c973e425c 100644 --- a/services/web/app/src/Features/Templates/TemplatesManager.js +++ b/services/web/app/src/Features/Templates/TemplatesManager.js @@ -4,7 +4,6 @@ const ProjectDetailsHandler = require('../Project/ProjectDetailsHandler') const ProjectOptionsHandler = require('../Project/ProjectOptionsHandler') const ProjectRootDocManager = require('../Project/ProjectRootDocManager') const ProjectUploadManager = require('../Uploads/ProjectUploadManager') -const FileWriter = require('../../infrastructure/FileWriter') const async = require('async') const fs = require('fs') const util = require('util') @@ -41,81 +40,76 @@ const TemplatesManager = { logger.warn({ err }, 'error getting zip from template API') return callback(err) }) - FileWriter.ensureDumpFolderExists(function (err) { - if (err) { - return callback(err) - } - const projectName = ProjectDetailsHandler.fixProjectName(templateName) - const dumpPath = `${settings.path.dumpFolder}/${crypto.randomUUID()}` - const writeStream = fs.createWriteStream(dumpPath) - const attributes = { - fromV1TemplateId: templateId, - fromV1TemplateVersionId: templateVersionId, - } - writeStream.on('close', function () { - if (zipReq.response.statusCode !== 200) { - logger.warn( - { uri: zipUrl, statusCode: zipReq.response.statusCode }, - 'non-success code getting zip from template API' - ) - return callback(new Error('get zip failed')) - } - ProjectUploadManager.createProjectFromZipArchiveWithName( - userId, - projectName, - dumpPath, - attributes, - function (err, project) { - if (err) { - OError.tag(err, 'problem building project from zip', { - zipReq, - }) - return callback(err) - } - async.series( - [ - cb => TemplatesManager._setCompiler(project._id, compiler, cb), - cb => TemplatesManager._setImage(project._id, imageName, cb), - cb => TemplatesManager._setMainFile(project._id, mainFile, cb), - cb => - TemplatesManager._setBrandVariationId( - project._id, - brandVariationId, - cb - ), - ], - function (err) { - if (err) { - return callback(err) - } - fs.unlink(dumpPath, function (err) { - if (err) { - return logger.err({ err }, 'error unlinking template zip') - } - }) - const update = { - fromV1TemplateId: templateId, - fromV1TemplateVersionId: templateVersionId, - } - Project.updateOne( - { _id: project._id }, - update, - {}, - function (err) { - if (err) { - return callback(err) - } - callback(null, project) - } - ) - } - ) - } + const projectName = ProjectDetailsHandler.fixProjectName(templateName) + const dumpPath = `${settings.path.dumpFolder}/${crypto.randomUUID()}` + const writeStream = fs.createWriteStream(dumpPath) + const attributes = { + fromV1TemplateId: templateId, + fromV1TemplateVersionId: templateVersionId, + } + writeStream.on('close', function () { + if (zipReq.response.statusCode !== 200) { + logger.warn( + { uri: zipUrl, statusCode: zipReq.response.statusCode }, + 'non-success code getting zip from template API' ) - }) - zipReq.pipe(writeStream) + return callback(new Error('get zip failed')) + } + ProjectUploadManager.createProjectFromZipArchiveWithName( + userId, + projectName, + dumpPath, + attributes, + function (err, project) { + if (err) { + OError.tag(err, 'problem building project from zip', { + zipReq, + }) + return callback(err) + } + async.series( + [ + cb => TemplatesManager._setCompiler(project._id, compiler, cb), + cb => TemplatesManager._setImage(project._id, imageName, cb), + cb => TemplatesManager._setMainFile(project._id, mainFile, cb), + cb => + TemplatesManager._setBrandVariationId( + project._id, + brandVariationId, + cb + ), + ], + function (err) { + if (err) { + return callback(err) + } + fs.unlink(dumpPath, function (err) { + if (err) { + return logger.err({ err }, 'error unlinking template zip') + } + }) + const update = { + fromV1TemplateId: templateId, + fromV1TemplateVersionId: templateVersionId, + } + Project.updateOne( + { _id: project._id }, + update, + {}, + function (err) { + if (err) { + return callback(err) + } + callback(null, project) + } + ) + } + ) + } + ) }) + zipReq.pipe(writeStream) }, _setCompiler(projectId, compiler, callback) { diff --git a/services/web/app/src/infrastructure/FileWriter.js b/services/web/app/src/infrastructure/FileWriter.js index 285ac9b0cd..f1d4d478e3 100644 --- a/services/web/app/src/infrastructure/FileWriter.js +++ b/services/web/app/src/infrastructure/FileWriter.js @@ -57,17 +57,8 @@ class SizeLimitedStream extends Transform { } const FileWriter = { - ensureDumpFolderExists(callback) { - if (callback == null) { - callback = function () {} - } - return fs.mkdir(Settings.path.dumpFolder, function (error) { - if (error != null && error.code !== 'EEXIST') { - // Ignore error about already existing - return callback(error) - } - return callback(null) - }) + ensureDumpFolderExists() { + fs.mkdirSync(Settings.path.dumpFolder, { recursive: true }) }, writeLinesToDisk(identifier, lines, callback) { @@ -81,20 +72,14 @@ const FileWriter = { if (callback == null) { callback = function () {} } - callback = _.once(callback) const fsPath = `${ Settings.path.dumpFolder }/${identifier}_${crypto.randomUUID()}` - return FileWriter.ensureDumpFolderExists(function (error) { + return fs.writeFile(fsPath, content, function (error) { if (error != null) { return callback(error) } - return fs.writeFile(fsPath, content, function (error) { - if (error != null) { - return callback(error) - } - return callback(null, fsPath) - }) + return callback(null, fsPath) }) }, @@ -112,57 +97,47 @@ const FileWriter = { Settings.path.dumpFolder }/${identifier}_${crypto.randomUUID()}` - stream.pause() + const writeStream = fs.createWriteStream(fsPath) + const passThrough = new SizeLimitedStream({ + maxSizeBytes: options.maxSizeBytes, + }) - FileWriter.ensureDumpFolderExists(function (error) { - const writeStream = fs.createWriteStream(fsPath) - - if (error != null) { - return callback(error) - } - stream.resume() - - const passThrough = new SizeLimitedStream({ - maxSizeBytes: options.maxSizeBytes, + // if writing fails, we want to consume the bytes from the source, to avoid leaks + for (const evt of ['error', 'close']) { + writeStream.on(evt, function () { + passThrough.unpipe(writeStream) + passThrough.resume() }) + } - // if writing fails, we want to consume the bytes from the source, to avoid leaks - for (const evt of ['error', 'close']) { - writeStream.on(evt, function () { - passThrough.unpipe(writeStream) - passThrough.resume() - }) + pipeline(stream, passThrough, writeStream, function (err) { + if ( + options.maxSizeBytes && + passThrough.bytes >= options.maxSizeBytes && + !(err instanceof FileTooLargeError) + ) { + err = new FileTooLargeError({ + message: 'stream size limit reached', + info: { size: passThrough.bytes }, + }).withCause(err || {}) } - - pipeline(stream, passThrough, writeStream, function (err) { - if ( - options.maxSizeBytes && - passThrough.bytes >= options.maxSizeBytes && - !(err instanceof FileTooLargeError) - ) { - err = new FileTooLargeError({ - message: 'stream size limit reached', - info: { size: passThrough.bytes }, - }).withCause(err || {}) - } - if (err) { - OError.tag( - err, - '[writeStreamToDisk] something went wrong writing the stream to disk', - { - identifier, - fsPath, - } - ) - return callback(err) - } - - logger.debug( - { identifier, fsPath }, - '[writeStreamToDisk] write stream finished' + if (err) { + OError.tag( + err, + '[writeStreamToDisk] something went wrong writing the stream to disk', + { + identifier, + fsPath, + } ) - callback(null, fsPath) - }) + return callback(err) + } + + logger.debug( + { identifier, fsPath }, + '[writeStreamToDisk] write stream finished' + ) + callback(null, fsPath) }) }, @@ -198,5 +173,7 @@ const FileWriter = { } module.exports = FileWriter -module.exports.promises = promisifyAll(FileWriter) +module.exports.promises = promisifyAll(FileWriter, { + without: ['ensureDumpFolderExists'], +}) module.exports.SizeLimitedStream = SizeLimitedStream diff --git a/services/web/test/unit/src/Templates/TemplatesManagerTests.js b/services/web/test/unit/src/Templates/TemplatesManagerTests.js index 0d8ae50b0d..af9436f15c 100644 --- a/services/web/test/unit/src/Templates/TemplatesManagerTests.js +++ b/services/web/test/unit/src/Templates/TemplatesManagerTests.js @@ -61,7 +61,6 @@ describe('TemplatesManager', function () { fixProjectName: sinon.stub().returns(this.templateName), } this.Project = { updateOne: sinon.stub().callsArgWith(3, null) } - this.FileWriter = { ensureDumpFolderExists: sinon.stub().callsArg(0) } this.FetchUtils = { fetchJson: sinon.stub(), RequestFailedError, @@ -76,7 +75,6 @@ describe('TemplatesManager', function () { '../Authentication/SessionManager': (this.SessionManager = { getLoggedInUserId: sinon.stub(), }), - '../../infrastructure/FileWriter': this.FileWriter, '@overleaf/settings': { path: { dumpFolder: this.dumpFolder, @@ -177,10 +175,6 @@ describe('TemplatesManager', function () { } ) }) - - it('should ensure that the dump folder exists', function () { - return sinon.assert.called(this.FileWriter.ensureDumpFolderExists) - }) }) describe('when some options not set', function () {