From 33ac9e18f9835ae53db36a2d039d70365274cc78 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 17 Aug 2023 10:09:26 +0200 Subject: [PATCH] Merge pull request #14358 from overleaf/jpa-abort-check [web] skip streaming clsi response when the request was aborted GitOrigin-RevId: 8152399f18328eafd3e93143190a5eb8d1fdff1d --- .../src/Features/Compile/CompileController.js | 76 +++++++++++++++++-- .../src/Compile/CompileControllerTests.js | 20 ++++- 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 67d018b485..59356b207a 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -328,7 +328,15 @@ module.exports = CompileController = { req.params.build_id, 'output.pdf' ) - CompileController.proxyToClsi(projectId, url, {}, req, res, next) + CompileController.proxyToClsi( + projectId, + 'output-file', + url, + {}, + req, + res, + next + ) }) } }) @@ -374,7 +382,15 @@ module.exports = CompileController = { return } const url = `/project/${projectId}/output/output.pdf` - CompileController.proxyToClsi(projectId, url, {}, req, res, next) + CompileController.proxyToClsi( + projectId, + 'output-file', + url, + {}, + req, + res, + next + ) }) }, @@ -390,7 +406,15 @@ module.exports = CompileController = { req.params.build_id, req.params.file ) - CompileController.proxyToClsi(projectId, url, {}, req, res, next) + CompileController.proxyToClsi( + projectId, + 'output-file', + url, + {}, + req, + res, + next + ) }) }, @@ -411,6 +435,7 @@ module.exports = CompileController = { } CompileController.proxyToClsiWithLimits( submissionId, + 'output-file', url, {}, limits, @@ -467,6 +492,7 @@ module.exports = CompileController = { const url = CompileController._getUrl(projectId, userId, 'sync/pdf') CompileController.proxyToClsi( projectId, + 'sync-to-pdf', url, { page, h, v, imageName }, req, @@ -508,6 +534,7 @@ module.exports = CompileController = { const url = CompileController._getUrl(projectId, userId, 'sync/code') CompileController.proxyToClsi( projectId, + 'sync-to-code', url, { file, line, column, imageName }, req, @@ -518,13 +545,14 @@ module.exports = CompileController = { }) }, - proxyToClsi(projectId, url, qs, req, res, next) { + proxyToClsi(projectId, action, url, qs, req, res, next) { CompileManager.getProjectCompileLimits(projectId, function (error, limits) { if (error) { return next(error) } CompileController.proxyToClsiWithLimits( projectId, + action, url, qs, limits, @@ -535,7 +563,7 @@ module.exports = CompileController = { }) }, - proxyToClsiWithLimits(projectId, url, qs, limits, req, res, next) { + proxyToClsiWithLimits(projectId, action, url, qs, limits, req, res, next) { _getPersistenceOptions( req, projectId, @@ -551,19 +579,43 @@ module.exports = CompileController = { ...persistenceOptions.qs, ...qs, }).toString() + const timer = new Metrics.Timer( + 'proxy_to_clsi', + 1, + { path: action }, + [0, 100, 1000, 2000, 5000, 10000, 15000, 20000, 30000, 45000, 60000] + ) + Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'start' }) fetchStreamWithResponse(url.href, { method: req.method, signal: AbortSignal.timeout(60 * 1000), headers: persistenceOptions.headers, }) .then(({ stream, response }) => { + if (req.destroyed) { + // The client has disconnected already, avoid trying to write into the broken connection. + Metrics.inc('proxy_to_clsi', 1, { + path: action, + status: 'req-aborted', + }) + return + } + Metrics.inc('proxy_to_clsi', 1, { + path: action, + status: response.status, + }) + for (const key of ['Content-Length', 'Content-Type']) { res.setHeader(key, response.headers.get(key)) } res.writeHead(response.status) return pipeline(stream, res) }) + .then(() => { + timer.done() + }) .catch(err => { + const duration = timer.done() if (!res.headersSent) { if (err instanceof RequestFailedError) { res.sendStatus(err.response.status) @@ -571,7 +623,19 @@ module.exports = CompileController = { res.sendStatus(500) } } - logger.warn({ err, projectId, url }, 'CLSI proxy error') + const reqAborted = Boolean(req.destroyed) + if (reqAborted) { + Metrics.inc('proxy_to_clsi', 1, { + path: action, + status: 'req-aborted-late', + }) + } else { + Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'error' }) + } + logger.warn( + { err, projectId, url, action, reqAborted, duration }, + 'CLSI proxy error' + ) }) } ) diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index 11529ea10c..eca488cf45 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -78,7 +78,12 @@ describe('CompileController', function () { '@overleaf/fetch-utils': this.fetchUtils, request: (this.request = sinon.stub()), '../Project/ProjectGetter': (this.ProjectGetter = {}), - '@overleaf/metrics': (this.Metrics = { inc: sinon.stub() }), + '@overleaf/metrics': (this.Metrics = { + inc: sinon.stub(), + Timer: class { + done() {} + }, + }), './CompileManager': this.CompileManager, '../User/UserGetter': this.UserGetter, './ClsiManager': this.ClsiManager, @@ -409,6 +414,7 @@ describe('CompileController', function () { this.CompileController.proxyToClsi .calledWith( this.projectId, + 'output-file', `/project/${this.projectId}/user/${this.user_id}/output/output.pdf`, {}, this.req, @@ -432,6 +438,7 @@ describe('CompileController', function () { this.CompileController.proxyToClsi .calledWith( this.projectId, + 'output-file', `/project/${this.projectId}/user/${this.user_id}/build/${this.buildId}/output/output.pdf`, {}, this.req, @@ -470,6 +477,7 @@ describe('CompileController', function () { it('should proxy to CLSI with correct URL and default limits', function () { this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith( this.submission_id, + 'output-file', this.expected_url, {}, { @@ -493,6 +501,7 @@ describe('CompileController', function () { it('should proxy to CLSI with correct URL and specified limits', function () { this.CompileController.proxyToClsiWithLimits.should.have.been.calledWith( this.submission_id, + 'output-file', this.expected_url, {}, { @@ -526,6 +535,7 @@ describe('CompileController', function () { it('should proxy the request with an imageName', function () { expect(this.CompileController.proxyToClsi).to.have.been.calledWith( this.projectId, + 'sync-to-code', `/project/${this.projectId}/user/${this.user_id}/sync/code`, { file, line, column, imageName }, this.req, @@ -558,6 +568,7 @@ describe('CompileController', function () { it('should proxy the request with an imageName', function () { expect(this.CompileController.proxyToClsi).to.have.been.calledWith( this.projectId, + 'sync-to-pdf', `/project/${this.projectId}/user/${this.user_id}/sync/pdf`, { page, h, v, imageName }, this.req, @@ -590,6 +601,7 @@ describe('CompileController', function () { }) this.CompileController.proxyToClsi( this.projectId, + 'output-file', (this.url = '/test'), { query: 'foo' }, this.req, @@ -620,6 +632,7 @@ describe('CompileController', function () { }) this.CompileController.proxyToClsi( this.projectId, + 'output-file', (this.url = '/test'), {}, this.req, @@ -647,6 +660,7 @@ describe('CompileController', function () { }) this.CompileController.proxyToClsi( this.projectId, + 'output-file', (this.url = '/test'), {}, this.req, @@ -678,6 +692,7 @@ describe('CompileController', function () { }) this.CompileController.proxyToClsi( this.projectId, + 'output-file', (this.url = '/test'), {}, this.req, @@ -705,6 +720,7 @@ describe('CompileController', function () { this.req.query = { build: 1234 } this.CompileController.proxyToClsi( this.projectId, + 'output-file', (this.url = '/test'), {}, this.req, @@ -765,6 +781,7 @@ describe('CompileController', function () { sinon.assert.calledWith( this.CompileController.proxyToClsi, this.projectId, + 'output-file', `/project/${this.projectId}/output/output.pdf`, {}, this.req, @@ -774,6 +791,7 @@ describe('CompileController', function () { this.CompileController.proxyToClsi .calledWith( this.projectId, + 'output-file', `/project/${this.projectId}/output/output.pdf`, {}, this.req,