From d12a0b5f07ed3bf816811805cd0a534d5bdc7d6f Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Thu, 11 Apr 2024 08:35:16 -0400 Subject: [PATCH] Merge pull request #17735 from overleaf/em-promisify-web-api-manager Promisify WebApiManager GitOrigin-RevId: 95addc9442845252aa51c353676486b2dbce0662 --- package-lock.json | 4 +- .../app/js/UpdatesProcessor.js | 3 +- .../project-history/app/js/WebApiManager.js | 160 +++++++++-------- services/project-history/package.json | 3 +- .../js/WebApiManager/WebApiManagerTests.js | 166 ++++++++---------- 5 files changed, 165 insertions(+), 171 deletions(-) diff --git a/package-lock.json b/package-lock.json index bc41b80046..406f7d891c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -43058,8 +43058,7 @@ "mongodb-legacy": "^6.0.1", "overleaf-editor-core": "*", "redis": "~0.10.1", - "request": "^2.88.2", - "requestretry": "^7.1.0" + "request": "^2.88.2" }, "devDependencies": { "chai": "^4.3.6", @@ -51663,7 +51662,6 @@ "overleaf-editor-core": "*", "redis": "~0.10.1", "request": "^2.88.2", - "requestretry": "^7.1.0", "sinon": "~9.0.1", "sinon-chai": "^3.7.0", "timekeeper": "2.2.0", diff --git a/services/project-history/app/js/UpdatesProcessor.js b/services/project-history/app/js/UpdatesProcessor.js index 841ceba39b..eb8a051ed1 100644 --- a/services/project-history/app/js/UpdatesProcessor.js +++ b/services/project-history/app/js/UpdatesProcessor.js @@ -237,7 +237,7 @@ export function _getHistoryId(projectId, updates, callback) { } } - WebApiManager.getHistoryId(projectId, (error, idFromWeb, cached) => { + WebApiManager.getHistoryId(projectId, (error, idFromWeb) => { if (error != null && idFromUpdates != null) { // present only on updates // 404s from web are an error @@ -266,7 +266,6 @@ export function _getHistoryId(projectId, updates, callback) { projectId, idFromWeb, idFromUpdates, - idWasCached: cached, updates, }, 'inconsistent project history id between updates and web' diff --git a/services/project-history/app/js/WebApiManager.js b/services/project-history/app/js/WebApiManager.js index bbe707b7b9..d007d0cdda 100644 --- a/services/project-history/app/js/WebApiManager.js +++ b/services/project-history/app/js/WebApiManager.js @@ -1,96 +1,104 @@ -import { promisify } from 'util' -import request from 'requestretry' // allow retry on error https://github.com/FGRibreau/node-request-retry +import { callbackify } from 'util' +import { setTimeout } from 'timers/promises' import logger from '@overleaf/logger' import Metrics from '@overleaf/metrics' -import OError from '@overleaf/o-error' import Settings from '@overleaf/settings' +import { + fetchNothing, + fetchJson, + RequestFailedError, +} from '@overleaf/fetch-utils' import * as Errors from './Errors.js' import * as RedisManager from './RedisManager.js' -// Don't let HTTP calls hang for a long time -const DEFAULT_MAX_HTTP_REQUEST_LENGTH = 16000 // 16 seconds +let RETRY_TIMEOUT_MS = 5000 -export function getHistoryId(projectId, callback) { +async function getHistoryId(projectId) { Metrics.inc('history_id_cache_requests_total') - RedisManager.getCachedHistoryId(projectId, (err, cachedHistoryId) => { - if (err) return callback(err) - if (cachedHistoryId) { - Metrics.inc('history_id_cache_hits_total') - callback(null, cachedHistoryId, true) + const cachedHistoryId = + await RedisManager.promises.getCachedHistoryId(projectId) + if (cachedHistoryId) { + Metrics.inc('history_id_cache_hits_total') + return cachedHistoryId + } else { + const project = await _getProjectDetails(projectId) + const historyId = + project.overleaf && + project.overleaf.history && + project.overleaf.history.id + if (historyId != null) { + await RedisManager.promises.setCachedHistoryId(projectId, historyId) + } + return historyId + } +} + +async function requestResync(projectId) { + try { + await fetchNothing( + `${Settings.apis.web.url}/project/${projectId}/history/resync`, + { + method: 'POST', + signal: AbortSignal.timeout(6 * 60000), + basicAuth: { + user: Settings.apis.web.user, + password: Settings.apis.web.pass, + }, + } + ) + } catch (err) { + if (err instanceof RequestFailedError && err.response.status === 404) { + throw new Errors.NotFoundError('got a 404 from web api').withCause(err) } else { - _getProjectDetails(projectId, function (error, project) { - if (error) { - return callback(error) - } - const historyId = - project.overleaf && - project.overleaf.history && - project.overleaf.history.id - if (historyId != null) { - RedisManager.setCachedHistoryId(projectId, historyId, err => { - if (err) return callback(err) - callback(null, historyId, false) - }) - } else { - callback(null, historyId, false) - } - }) + throw err } - }) + } } -export function requestResync(projectId, callback) { - const path = `/project/${projectId}/history/resync` - _sendRequest( - { path, timeout: 6 * 60000, maxAttempts: 1, method: 'POST' }, - callback - ) -} - -function _getProjectDetails(projectId, callback) { - const path = `/project/${projectId}/details` +async function _getProjectDetails(projectId, callback) { logger.debug({ projectId }, 'getting project details from web') - _sendRequest({ path, json: true }, callback) -} - -function _sendRequest(options, callback) { - const url = `${Settings.apis.web.url}${options.path}` - request( - { - method: options.method || 'GET', - url, - json: options.json || false, - timeout: options.timeout || DEFAULT_MAX_HTTP_REQUEST_LENGTH, - maxAttempts: options.maxAttempts || 2, // for node-request-retry - auth: { - user: Settings.apis.web.user, - pass: Settings.apis.web.pass, - sendImmediately: true, - }, - }, - function (error, res, body) { - if (error != null) { - return callback(OError.tag(error)) - } - if (res.statusCode === 404) { - logger.debug({ url }, 'got 404 from web api') - error = new Errors.NotFoundError('got a 404 from web api') - return callback(error) - } - if (res.statusCode >= 200 && res.statusCode < 300) { - callback(null, body) + let attempts = 0 + while (true) { + attempts += 1 + try { + return await fetchJson( + `${Settings.apis.web.url}/project/${projectId}/details`, + { + signal: AbortSignal.timeout(16000), + basicAuth: { + user: Settings.apis.web.user, + password: Settings.apis.web.pass, + }, + } + ) + } catch (err) { + if (err instanceof RequestFailedError && err.response.status === 404) { + throw new Errors.NotFoundError('got a 404 from web api').withCause(err) + } else if (attempts < 2) { + // retry after 5 seconds + await setTimeout(RETRY_TIMEOUT_MS) } else { - error = new OError( - `web returned a non-success status code: ${res.statusCode} (attempts: ${res.attempts})`, - { url, res } - ) - callback(error) + throw err } } - ) + } } +/** + * Adjust the retry timeout in tests + */ +export async function setRetryTimeoutMs(timeoutMs) { + RETRY_TIMEOUT_MS = timeoutMs +} + +// EXPORTS + +const getHistoryIdCb = callbackify(getHistoryId) +const requestResyncCb = callbackify(requestResync) + +export { getHistoryIdCb as getHistoryId, requestResyncCb as requestResync } + export const promises = { - getHistoryId: promisify(getHistoryId), - requestResync: promisify(requestResync), + getHistoryId, + requestResync, } diff --git a/services/project-history/package.json b/services/project-history/package.json index 5c21d05713..1def05f55a 100644 --- a/services/project-history/package.json +++ b/services/project-history/package.json @@ -43,8 +43,7 @@ "mongodb-legacy": "^6.0.1", "overleaf-editor-core": "*", "redis": "~0.10.1", - "request": "^2.88.2", - "requestretry": "^7.1.0" + "request": "^2.88.2" }, "devDependencies": { "chai": "^4.3.6", diff --git a/services/project-history/test/unit/js/WebApiManager/WebApiManagerTests.js b/services/project-history/test/unit/js/WebApiManager/WebApiManagerTests.js index 4506a0290f..7a9c795ed3 100644 --- a/services/project-history/test/unit/js/WebApiManager/WebApiManagerTests.js +++ b/services/project-history/test/unit/js/WebApiManager/WebApiManagerTests.js @@ -1,13 +1,12 @@ -import async from 'async' import sinon from 'sinon' import { expect } from 'chai' import { strict as esmock } from 'esmock' +import { RequestFailedError } from '@overleaf/fetch-utils' const MODULE_PATH = '../../../../app/js/WebApiManager.js' describe('WebApiManager', function () { beforeEach(async function () { - this.request = sinon.stub() this.settings = { apis: { web: { @@ -17,146 +16,137 @@ describe('WebApiManager', function () { }, }, } - this.callback = sinon.stub() this.userId = 'mock-user-id' this.projectId = 'mock-project-id' this.project = { features: 'mock-features' } this.olProjectId = 12345 this.Metrics = { inc: sinon.stub() } this.RedisManager = { - getCachedHistoryId: sinon.stub(), - setCachedHistoryId: sinon.stub().yields(), + promises: { + getCachedHistoryId: sinon.stub(), + setCachedHistoryId: sinon.stub().resolves(), + }, + } + this.FetchUtils = { + fetchNothing: sinon.stub().resolves(), + fetchJson: sinon.stub(), + RequestFailedError, } this.WebApiManager = await esmock(MODULE_PATH, { - requestretry: this.request, + '@overleaf/fetch-utils': this.FetchUtils, '@overleaf/settings': this.settings, '@overleaf/metrics': this.Metrics, '../../../../app/js/RedisManager.js': this.RedisManager, }) + this.WebApiManager.setRetryTimeoutMs(100) }) describe('getHistoryId', function () { describe('when there is no cached value and the web request is successful', function () { beforeEach(function () { - this.RedisManager.getCachedHistoryId + this.RedisManager.promises.getCachedHistoryId .withArgs(this.projectId) // first call, no cached value returned .onCall(0) - .yields() - this.RedisManager.getCachedHistoryId + .resolves(null) + this.RedisManager.promises.getCachedHistoryId .withArgs(this.projectId) // subsequent calls, return cached value - .yields(null, this.olProjectId) - this.RedisManager.getCachedHistoryId + .resolves(this.olProjectId) + this.RedisManager.promises.getCachedHistoryId .withArgs('mock-project-id-2') // no cached value for other project - .yields() - this.request.yields( - null, - { statusCode: 200 }, - { overleaf: { history: { id: this.olProjectId } } } - ) + .resolves(null) + this.FetchUtils.fetchJson.resolves({ + overleaf: { history: { id: this.olProjectId } }, + }) }) - it('should only request project details once per project', function (done) { - async.times( - 5, - (n, cb) => { - this.WebApiManager.getHistoryId(this.projectId, cb) - }, - () => { - this.request.callCount.should.equal(1) + it('should only request project details once per project', async function () { + for (let i = 0; i < 5; i++) { + await this.WebApiManager.promises.getHistoryId(this.projectId) + } + this.FetchUtils.fetchJson.should.have.been.calledOnce - this.WebApiManager.getHistoryId('mock-project-id-2', () => { - this.request.callCount.should.equal(2) - done() - }) - } - ) + await this.WebApiManager.promises.getHistoryId('mock-project-id-2') + this.FetchUtils.fetchJson.should.have.been.calledTwice }) - it('should cache the history id', function (done) { - this.WebApiManager.getHistoryId( - this.projectId, - (error, olProjectId) => { - if (error) return done(error) - this.RedisManager.setCachedHistoryId - .calledWith(this.projectId, olProjectId) - .should.equal(true) - done() - } + it('should cache the history id', async function () { + const olProjectId = await this.WebApiManager.promises.getHistoryId( + this.projectId ) + this.RedisManager.promises.setCachedHistoryId + .calledWith(this.projectId, olProjectId) + .should.equal(true) }) - it('should call the callback with the project', function (done) { - this.WebApiManager.getHistoryId( - this.projectId, - (error, olProjectId) => { - expect(error).to.be.null - expect( - this.request.calledWithMatch({ - method: 'GET', - url: `${this.settings.apis.web.url}/project/${this.projectId}/details`, - json: true, - auth: { - user: this.settings.apis.web.user, - pass: this.settings.apis.web.pass, - sendImmediately: true, - }, - }) - ).to.be.true - expect(olProjectId).to.equal(this.olProjectId) - done() + it("should return the project's history id", async function () { + const olProjectId = await this.WebApiManager.promises.getHistoryId( + this.projectId + ) + + expect(this.FetchUtils.fetchJson).to.have.been.calledWithMatch( + `${this.settings.apis.web.url}/project/${this.projectId}/details`, + { + basicAuth: { + user: this.settings.apis.web.user, + password: this.settings.apis.web.pass, + }, } ) + expect(olProjectId).to.equal(this.olProjectId) }) }) describe('when the web API returns an error', function () { beforeEach(function () { this.error = new Error('something went wrong') - this.request.yields(this.error) - this.RedisManager.getCachedHistoryId.yields() - this.WebApiManager.getHistoryId(this.projectId, this.callback) + this.FetchUtils.fetchJson.rejects(this.error) + this.RedisManager.promises.getCachedHistoryId.resolves(null) }) - it('should return an error to the callback', function () { - this.callback.calledWith(this.error).should.equal(true) + it('should throw an error', async function () { + await expect( + this.WebApiManager.promises.getHistoryId(this.projectId) + ).to.be.rejectedWith(this.error) }) }) describe('when web returns a 404', function () { beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 404 }, '') - this.RedisManager.getCachedHistoryId.yields() - this.WebApiManager.getHistoryId(this.projectId, this.callback) + this.FetchUtils.fetchJson.rejects( + new RequestFailedError( + 'http://some-url', + {}, + { status: 404 }, + 'Not found' + ) + ) + this.RedisManager.promises.getCachedHistoryId.resolves(null) }) - it('should return the callback with an error', function () { - this.callback - .calledWith(sinon.match.has('message', 'got a 404 from web api')) - .should.equal(true) + it('should throw an error', async function () { + await expect( + this.WebApiManager.promises.getHistoryId(this.projectId) + ).to.be.rejectedWith('got a 404 from web api') }) }) describe('when web returns a failure error code', function () { beforeEach(function () { - this.RedisManager.getCachedHistoryId.yields() - this.request.callsArgWith( - 1, - null, - { statusCode: 500, attempts: 42 }, - '' + this.RedisManager.promises.getCachedHistoryId.resolves(null) + this.FetchUtils.fetchJson.rejects( + new RequestFailedError( + 'http://some-url', + {}, + { status: 500 }, + 'Error' + ) ) - this.WebApiManager.getHistoryId(this.projectId, this.callback) }) - it('should return the callback with an error', function () { - this.callback - .calledWith( - sinon.match.has( - 'message', - 'web returned a non-success status code: 500 (attempts: 42)' - ) - ) - .should.equal(true) + it('should throw an error', async function () { + await expect( + this.WebApiManager.promises.getHistoryId(this.projectId) + ).to.be.rejectedWith(RequestFailedError) }) }) })