From 8136036c3377ab77e71d2b674f74d30e2bb9023e Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 30 Jan 2024 10:35:54 -0500 Subject: [PATCH] Merge pull request #16644 from overleaf/em-promisify-update-manager Promisify UpdateManager GitOrigin-RevId: 2c3e21ee6ef2454f79695ca8623c3d38720ff6bf --- libraries/metrics/index.js | 2 +- libraries/redis-wrapper/RedisLocker.js | 22 + package-lock.json | 24 +- .../app/js/DocumentManager.js | 46 +- .../document-updater/app/js/HistoryManager.js | 13 +- .../document-updater/app/js/LockManager.js | 8 - .../app/js/PersistenceManager.js | 22 +- .../app/js/ProjectHistoryRedisManager.js | 7 +- .../document-updater/app/js/RangesManager.js | 12 +- .../app/js/RealTimeRedisManager.js | 9 +- .../document-updater/app/js/RedisManager.js | 9 +- .../app/js/ShareJsUpdateManager.js | 12 +- .../app/js/SnapshotManager.js | 9 +- .../document-updater/app/js/UpdateManager.js | 544 +++++++---------- services/document-updater/package.json | 1 + services/document-updater/test/setup.js | 5 +- .../js/UpdateManager/UpdateManagerTests.js | 548 +++++++++--------- 17 files changed, 648 insertions(+), 645 deletions(-) diff --git a/libraries/metrics/index.js b/libraries/metrics/index.js index b711ea4a49..c4cc2b8f3e 100644 --- a/libraries/metrics/index.js +++ b/libraries/metrics/index.js @@ -97,7 +97,7 @@ function histogram(key, value, buckets, labels = {}) { } class Timer { - constructor(key, sampleRate = 1, labels = {}, buckets) { + constructor(key, sampleRate = 1, labels = {}, buckets = undefined) { if (typeof sampleRate === 'object') { // called with (key, labels, buckets) if (arguments.length === 3) { diff --git a/libraries/redis-wrapper/RedisLocker.js b/libraries/redis-wrapper/RedisLocker.js index 03f861e95b..20598e33e5 100644 --- a/libraries/redis-wrapper/RedisLocker.js +++ b/libraries/redis-wrapper/RedisLocker.js @@ -1,3 +1,4 @@ +const { promisify } = require('util') const metrics = require('@overleaf/metrics') const logger = require('@overleaf/logger') const os = require('os') @@ -64,6 +65,27 @@ module.exports = class RedisLocker { // read-only copy for unit tests this.unlockScript = UNLOCK_SCRIPT + + this.promises = { + checkLock: promisify(this.checkLock.bind(this)), + getLock: promisify(this.getLock.bind(this)), + releaseLock: promisify(this.releaseLock.bind(this)), + + // tryLock returns two values: gotLock and lockValue. We need to merge + // these two values into one for the promises version. + tryLock: id => + new Promise((resolve, reject) => { + this.tryLock(id, (err, gotLock, lockValue) => { + if (err) { + reject(err) + } else if (!gotLock) { + resolve(null) + } else { + resolve(lockValue) + } + }) + }), + } } // Use a signed lock value as described in diff --git a/package-lock.json b/package-lock.json index 18e059f6b6..c5c6a6e176 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15503,8 +15503,15 @@ "node_modules/@types/chai": { "version": "4.3.0", "resolved": "https://registry.npmjs.org/@types/chai/-/chai-4.3.0.tgz", - "integrity": "sha512-/ceqdqeRraGolFTcfoXNiqjyQhZzbINDngeoAq9GoHa8PPK1yNzTaxWjA6BFWp5Ua9JpXEMSS4s5i9tS0hOJtw==", - "dev": true + "integrity": "sha512-/ceqdqeRraGolFTcfoXNiqjyQhZzbINDngeoAq9GoHa8PPK1yNzTaxWjA6BFWp5Ua9JpXEMSS4s5i9tS0hOJtw==" + }, + "node_modules/@types/chai-as-promised": { + "version": "7.1.8", + "resolved": "https://registry.npmjs.org/@types/chai-as-promised/-/chai-as-promised-7.1.8.tgz", + "integrity": "sha512-ThlRVIJhr69FLlh6IctTXFkmhtP3NpMZ2QGq69StYLyKZFp/HOp1VdKZj7RvfNWYYcJ1xlbLGLLWj1UvP5u/Gw==", + "dependencies": { + "@types/chai": "*" + } }, "node_modules/@types/connect": { "version": "3.4.35", @@ -43700,6 +43707,7 @@ "@overleaf/ranges-tracker": "*", "@overleaf/redis-wrapper": "*", "@overleaf/settings": "*", + "@types/chai-as-promised": "^7.1.8", "async": "^3.2.2", "body-parser": "^1.19.0", "bunyan": "^1.8.15", @@ -54201,6 +54209,7 @@ "@overleaf/ranges-tracker": "*", "@overleaf/redis-wrapper": "*", "@overleaf/settings": "*", + "@types/chai-as-promised": "^7.1.8", "async": "^3.2.2", "body-parser": "^1.19.0", "bunyan": "^1.8.15", @@ -61468,8 +61477,15 @@ "@types/chai": { "version": "4.3.0", "resolved": "https://registry.npmjs.org/@types/chai/-/chai-4.3.0.tgz", - "integrity": "sha512-/ceqdqeRraGolFTcfoXNiqjyQhZzbINDngeoAq9GoHa8PPK1yNzTaxWjA6BFWp5Ua9JpXEMSS4s5i9tS0hOJtw==", - "dev": true + "integrity": "sha512-/ceqdqeRraGolFTcfoXNiqjyQhZzbINDngeoAq9GoHa8PPK1yNzTaxWjA6BFWp5Ua9JpXEMSS4s5i9tS0hOJtw==" + }, + "@types/chai-as-promised": { + "version": "7.1.8", + "resolved": "https://registry.npmjs.org/@types/chai-as-promised/-/chai-as-promised-7.1.8.tgz", + "integrity": "sha512-ThlRVIJhr69FLlh6IctTXFkmhtP3NpMZ2QGq69StYLyKZFp/HOp1VdKZj7RvfNWYYcJ1xlbLGLLWj1UvP5u/Gw==", + "requires": { + "@types/chai": "*" + } }, "@types/connect": { "version": "3.4.35", diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 6044f1e31f..4c20bcbbf5 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -1,4 +1,4 @@ -let DocumentManager +const { promisifyAll } = require('@overleaf/promise-utils') const RedisManager = require('./RedisManager') const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') const PersistenceManager = require('./PersistenceManager') @@ -11,7 +11,7 @@ const RangesManager = require('./RangesManager') const MAX_UNFLUSHED_AGE = 300 * 1000 // 5 mins, document should be flushed to mongo this time after a change -module.exports = DocumentManager = { +const DocumentManager = { getDoc(projectId, docId, _callback) { const timer = new Metrics.Timer('docManager.getDoc') const callback = (...args) => { @@ -680,3 +680,45 @@ module.exports = DocumentManager = { ) }, } + +module.exports = DocumentManager +module.exports.promises = promisifyAll(DocumentManager, { + multiResult: { + getDoc: [ + 'lines', + 'version', + 'ranges', + 'pathname', + 'projectHistoryId', + 'unflushedTime', + 'alreadyLoaded', + ], + getDocWithLock: [ + 'lines', + 'version', + 'ranges', + 'pathname', + 'projectHistoryId', + 'unflushedTime', + 'alreadyLoaded', + ], + getDocAndFlushIfOld: ['lines', 'version'], + getDocAndFlushIfOldWithLock: ['lines', 'version'], + getDocAndRecentOps: [ + 'lines', + 'version', + 'ops', + 'ranges', + 'pathname', + 'projectHistoryId', + ], + getDocAndRecentOpsWithLock: [ + 'lines', + 'version', + 'ops', + 'ranges', + 'pathname', + 'projectHistoryId', + ], + }, +}) diff --git a/services/document-updater/app/js/HistoryManager.js b/services/document-updater/app/js/HistoryManager.js index 5597498bfd..f7fca7d69e 100644 --- a/services/document-updater/app/js/HistoryManager.js +++ b/services/document-updater/app/js/HistoryManager.js @@ -1,12 +1,12 @@ -let HistoryManager const async = require('async') const logger = require('@overleaf/logger') +const { promisifyAll } = require('@overleaf/promise-utils') const request = require('request') const Settings = require('@overleaf/settings') const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') const metrics = require('./Metrics') -module.exports = HistoryManager = { +const HistoryManager = { // flush changes in the background flushProjectChangesAsync(projectId) { HistoryManager.flushProjectChanges( @@ -122,3 +122,12 @@ module.exports = HistoryManager = { ) }, } + +module.exports = HistoryManager +module.exports.promises = promisifyAll(HistoryManager, { + without: [ + 'flushProjectChangesAsync', + 'recordAndFlushHistoryOps', + 'shouldFlushHistoryOps', + ], +}) diff --git a/services/document-updater/app/js/LockManager.js b/services/document-updater/app/js/LockManager.js index 7b81101d4f..a65ab9f1e3 100644 --- a/services/document-updater/app/js/LockManager.js +++ b/services/document-updater/app/js/LockManager.js @@ -3,7 +3,6 @@ const redis = require('@overleaf/redis-wrapper') const rclient = redis.createClient(Settings.redis.lock) const keys = Settings.redis.lock.key_schema const RedisLocker = require('@overleaf/redis-wrapper/RedisLocker') -const { promisify } = require('@overleaf/promise-utils') module.exports = new RedisLocker({ rclient, @@ -17,10 +16,3 @@ module.exports = new RedisLocker({ metricsPrefix: 'doc', lockTTLSeconds: Settings.redisLockTTLSeconds, }) - -module.exports.promises = { - checkLock: promisify(module.exports.checkLock.bind(module.exports)), - getLock: promisify(module.exports.getLock.bind(module.exports)), - releaseLock: promisify(module.exports.releaseLock.bind(module.exports)), - tryLock: promisify(module.exports.tryLock.bind(module.exports)), -} diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index efaf586946..34e2ff89b9 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -1,8 +1,9 @@ +const { promisify } = require('util') +const { promisifyMultiResult } = require('@overleaf/promise-utils') const Settings = require('@overleaf/settings') const Errors = require('./Errors') const Metrics = require('./Metrics') const logger = require('@overleaf/logger') -const { promisifyAll } = require('@overleaf/promise-utils') const request = require('requestretry').defaults({ maxAttempts: 2, retryDelay: 10, @@ -175,10 +176,17 @@ function setDoc( ) } -module.exports = { getDoc, setDoc } - -module.exports.promises = promisifyAll(module.exports, { - multiResult: { - getDoc: ['lines', 'version', 'ranges', 'pathname', 'projectHistoryId'], +module.exports = { + getDoc, + setDoc, + promises: { + getDoc: promisifyMultiResult(getDoc, [ + 'lines', + 'version', + 'ranges', + 'pathname', + 'projectHistoryId', + ]), + setDoc: promisify(setDoc), }, -}) +} diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index 3b160079e8..47c8386383 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js @@ -1,5 +1,5 @@ -let ProjectHistoryRedisManager const Settings = require('@overleaf/settings') +const { promisifyAll } = require('@overleaf/promise-utils') const projectHistoryKeys = Settings.redis?.project_history?.key_schema const rclient = require('@overleaf/redis-wrapper').createClient( Settings.redis.project_history @@ -8,7 +8,7 @@ const logger = require('@overleaf/logger') const metrics = require('./Metrics') const { docIsTooLarge } = require('./Limits') -module.exports = ProjectHistoryRedisManager = { +const ProjectHistoryRedisManager = { queueOps(projectId, ...rest) { // Record metric for ops pushed onto queue const callback = rest.pop() @@ -172,3 +172,6 @@ module.exports = ProjectHistoryRedisManager = { ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate, callback) }, } + +module.exports = ProjectHistoryRedisManager +module.exports.promises = promisifyAll(ProjectHistoryRedisManager) diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index e2b2a31ca6..b87899d2f3 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -7,7 +7,7 @@ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -let RangesManager +const { promisifyAll } = require('@overleaf/promise-utils') const RangesTracker = require('@overleaf/ranges-tracker') const logger = require('@overleaf/logger') const Metrics = require('./Metrics') @@ -15,7 +15,7 @@ const _ = require('lodash') const RANGE_DELTA_BUCKETS = [0, 1, 2, 3, 4, 5, 10, 20, 50] -module.exports = RangesManager = { +const RangesManager = { MAX_COMMENTS: 500, MAX_CHANGES: 2000, @@ -176,3 +176,11 @@ module.exports = RangesManager = { return [emptyCount, totalCount] }, } + +module.exports = RangesManager +module.exports.promises = promisifyAll(RangesManager, { + without: ['_getRanges', '_emptyRangesCount'], + multiResult: { + applyUpdate: ['newRanges', 'rangesWereCollapsed'], + }, +}) diff --git a/services/document-updater/app/js/RealTimeRedisManager.js b/services/document-updater/app/js/RealTimeRedisManager.js index 27dd0b40df..32f15a546d 100644 --- a/services/document-updater/app/js/RealTimeRedisManager.js +++ b/services/document-updater/app/js/RealTimeRedisManager.js @@ -10,8 +10,8 @@ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -let RealTimeRedisManager const Settings = require('@overleaf/settings') +const { promisifyAll } = require('@overleaf/promise-utils') const rclient = require('@overleaf/redis-wrapper').createClient( Settings.redis.documentupdater ) @@ -30,7 +30,7 @@ let COUNT = 0 const MAX_OPS_PER_ITERATION = 8 // process a limited number of ops for safety -module.exports = RealTimeRedisManager = { +const RealTimeRedisManager = { getPendingUpdatesForDoc(docId, callback) { const multi = rclient.multi() multi.lrange( @@ -92,3 +92,8 @@ module.exports = RealTimeRedisManager = { } }, } + +module.exports = RealTimeRedisManager +module.exports.promises = promisifyAll(RealTimeRedisManager, { + without: ['sendData'], +}) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index e7f84a18ab..d69e11e103 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -1,16 +1,15 @@ -let RedisManager const Settings = require('@overleaf/settings') const rclient = require('@overleaf/redis-wrapper').createClient( Settings.redis.documentupdater ) const logger = require('@overleaf/logger') +const { promisifyAll } = require('@overleaf/promise-utils') const metrics = require('./Metrics') const Errors = require('./Errors') const crypto = require('crypto') const async = require('async') const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') const { docIsTooLarge } = require('./Limits') -const { promisifyAll } = require('@overleaf/promise-utils') // Sometimes Redis calls take an unexpectedly long time. We have to be // quick with Redis calls because we're holding a lock that expires @@ -28,7 +27,7 @@ const MAX_RANGES_SIZE = 3 * MEGABYTES const keys = Settings.redis.documentupdater.key_schema -module.exports = RedisManager = { +const RedisManager = { rclient, putDocInMemory( @@ -619,7 +618,9 @@ module.exports = RedisManager = { }, } -module.exports.promises = promisifyAll(module.exports, { +module.exports = RedisManager +module.exports.promises = promisifyAll(RedisManager, { + without: ['_deserializeRanges', '_computeHash'], multiResult: { getDoc: [ 'lines', diff --git a/services/document-updater/app/js/ShareJsUpdateManager.js b/services/document-updater/app/js/ShareJsUpdateManager.js index 2250973b9b..40a1e3b041 100644 --- a/services/document-updater/app/js/ShareJsUpdateManager.js +++ b/services/document-updater/app/js/ShareJsUpdateManager.js @@ -10,11 +10,11 @@ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -let ShareJsUpdateManager const ShareJsModel = require('./sharejs/server/model') const ShareJsDB = require('./ShareJsDB') const logger = require('@overleaf/logger') const Settings = require('@overleaf/settings') +const { promisifyAll } = require('@overleaf/promise-utils') const Keys = require('./UpdateKeys') const { EventEmitter } = require('events') const util = require('util') @@ -28,7 +28,7 @@ util.inherits(ShareJsModel, EventEmitter) const MAX_AGE_OF_OP = 80 -module.exports = ShareJsUpdateManager = { +const ShareJsUpdateManager = { getNewShareJsModel(projectId, docId, lines, version) { const db = new ShareJsDB(projectId, docId, lines, version) const model = new ShareJsModel(db, { @@ -143,3 +143,11 @@ module.exports = ShareJsUpdateManager = { .digest('hex') }, } + +module.exports = ShareJsUpdateManager +module.exports.promises = promisifyAll(ShareJsUpdateManager, { + without: ['getNewShareJsModel', '_listenForOps', '_sendOp', '_computeHash'], + multiResult: { + applyUpdate: ['updatedDocLines', 'version', 'appliedOps'], + }, +}) diff --git a/services/document-updater/app/js/SnapshotManager.js b/services/document-updater/app/js/SnapshotManager.js index 194e043549..c207e735e5 100644 --- a/services/document-updater/app/js/SnapshotManager.js +++ b/services/document-updater/app/js/SnapshotManager.js @@ -10,10 +10,10 @@ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -let SnapshotManager +const { promisifyAll } = require('@overleaf/promise-utils') const { db, ObjectId } = require('./mongodb') -module.exports = SnapshotManager = { +const SnapshotManager = { recordSnapshot(projectId, docId, version, pathname, lines, ranges, callback) { try { projectId = new ObjectId(projectId) @@ -76,3 +76,8 @@ module.exports = SnapshotManager = { } }, } + +module.exports = SnapshotManager +module.exports.promises = promisifyAll(SnapshotManager, { + without: ['jsonRangesToMongo', '_safeObjectId'], +}) diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index 06aa56b957..a53f294df0 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -1,24 +1,12 @@ -/* eslint-disable - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS201: Simplify complex destructure assignments - * DS205: Consider reworking code to avoid use of IIFEs - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -let UpdateManager +// @ts-check + +const { callbackifyAll } = require('@overleaf/promise-utils') const LockManager = require('./LockManager') const RedisManager = require('./RedisManager') const RealTimeRedisManager = require('./RealTimeRedisManager') const ShareJsUpdateManager = require('./ShareJsUpdateManager') const HistoryManager = require('./HistoryManager') -const Settings = require('@overleaf/settings') const _ = require('lodash') -const async = require('async') const logger = require('@overleaf/logger') const Metrics = require('./Metrics') const Errors = require('./Errors') @@ -27,337 +15,228 @@ const RangesManager = require('./RangesManager') const SnapshotManager = require('./SnapshotManager') const Profiler = require('./Profiler') -module.exports = UpdateManager = { - processOutstandingUpdates(projectId, docId, callback) { - if (!callback) { - callback = function () {} - } +const UpdateManager = { + async processOutstandingUpdates(projectId, docId) { const timer = new Metrics.Timer('updateManager.processOutstandingUpdates') - UpdateManager.fetchAndApplyUpdates(projectId, docId, function (error) { - timer.done() - callback(error) - }) + try { + await UpdateManager.fetchAndApplyUpdates(projectId, docId) + timer.done({ status: 'success' }) + } catch (err) { + timer.done({ status: 'error' }) + throw err + } }, - processOutstandingUpdatesWithLock(projectId, docId, callback) { - if (!callback) { - callback = function () {} - } + async processOutstandingUpdatesWithLock(projectId, docId) { const profile = new Profiler('processOutstandingUpdatesWithLock', { project_id: projectId, doc_id: docId, }) - LockManager.tryLock(docId, (error, gotLock, lockValue) => { - if (error) { - return callback(error) - } - if (!gotLock) { - return callback() - } - profile.log('tryLock') - UpdateManager.processOutstandingUpdates( - projectId, - docId, - function (error) { - if (error) { - return UpdateManager._handleErrorInsideLock( - docId, - lockValue, - error, - callback - ) - } - profile.log('processOutstandingUpdates') - LockManager.releaseLock(docId, lockValue, error => { - if (error) { - return callback(error) - } - profile.log('releaseLock').end() - UpdateManager.continueProcessingUpdatesWithLock( - projectId, - docId, - callback - ) - }) - } - ) - }) + + const lockValue = await LockManager.promises.tryLock(docId) + if (lockValue == null) { + return + } + profile.log('tryLock') + + try { + await UpdateManager.processOutstandingUpdates(projectId, docId) + profile.log('processOutstandingUpdates') + } finally { + await LockManager.promises.releaseLock(docId, lockValue) + profile.log('releaseLock').end() + } + + await UpdateManager.continueProcessingUpdatesWithLock(projectId, docId) }, - continueProcessingUpdatesWithLock(projectId, docId, callback) { - if (!callback) { - callback = function () {} + async continueProcessingUpdatesWithLock(projectId, docId) { + const length = await RealTimeRedisManager.promises.getUpdatesLength(docId) + if (length > 0) { + await UpdateManager.processOutstandingUpdatesWithLock(projectId, docId) } - RealTimeRedisManager.getUpdatesLength(docId, (error, length) => { - if (error) { - return callback(error) - } - if (length > 0) { - UpdateManager.processOutstandingUpdatesWithLock( - projectId, - docId, - callback - ) - } else { - callback() - } - }) }, - fetchAndApplyUpdates(projectId, docId, callback) { - if (!callback) { - callback = function () {} - } + async fetchAndApplyUpdates(projectId, docId) { const profile = new Profiler('fetchAndApplyUpdates', { project_id: projectId, doc_id: docId, }) - RealTimeRedisManager.getPendingUpdatesForDoc(docId, (error, updates) => { - if (error) { - return callback(error) - } - logger.debug( - { projectId, docId, count: updates.length }, - 'processing updates' - ) - if (updates.length === 0) { - return callback() - } - profile.log('getPendingUpdatesForDoc') - const doUpdate = (update, cb) => - UpdateManager.applyUpdate(projectId, docId, update, function (err) { - profile.log('applyUpdate') - cb(err) - }) - const finalCallback = function (err) { - profile.log('async done').end() - callback(err) - } - async.eachSeries(updates, doUpdate, finalCallback) - }) + + const updates = await RealTimeRedisManager.promises.getPendingUpdatesForDoc( + docId + ) + logger.debug( + { projectId, docId, count: updates.length }, + 'processing updates' + ) + if (updates.length === 0) { + return + } + profile.log('getPendingUpdatesForDoc') + + for (const update of updates) { + await UpdateManager.applyUpdate(projectId, docId, update) + profile.log('applyUpdate') + } + profile.log('async done').end() }, - applyUpdate(projectId, docId, update, _callback) { - if (_callback == null) { - _callback = function () {} - } - const callback = function (error) { - if (error) { - RealTimeRedisManager.sendData({ - project_id: projectId, - doc_id: docId, - error: error.message || error, - }) - profile.log('sendData') - } - profile.end() - _callback(error) - } - + async applyUpdate(projectId, docId, update) { const profile = new Profiler('applyUpdate', { project_id: projectId, doc_id: docId, }) + UpdateManager._sanitizeUpdate(update) profile.log('sanitizeUpdate', { sync: true }) - DocumentManager.getDoc( - projectId, - docId, - function (error, lines, version, ranges, pathname, projectHistoryId) { - profile.log('getDoc') - if (error) { - return callback(error) - } - if (lines == null || version == null) { - return callback( - new Errors.NotFoundError(`document not found: ${docId}`) - ) - } - const previousVersion = version - const incomingUpdateVersion = update.v - ShareJsUpdateManager.applyUpdate( + + try { + let { lines, version, ranges, pathname, projectHistoryId } = + await DocumentManager.promises.getDoc(projectId, docId) + profile.log('getDoc') + + if (lines == null || version == null) { + throw new Errors.NotFoundError(`document not found: ${docId}`) + } + + const previousVersion = version + const incomingUpdateVersion = update.v + let updatedDocLines, appliedOps + ;({ updatedDocLines, version, appliedOps } = + await ShareJsUpdateManager.promises.applyUpdate( projectId, docId, update, lines, - version, - function (error, updatedDocLines, version, appliedOps) { - profile.log('sharejs.applyUpdate', { - // only synchronous when the update applies directly to the - // doc version, otherwise getPreviousDocOps is called. - sync: incomingUpdateVersion === previousVersion, - }) - if (error) { - return callback(error) - } - RangesManager.applyUpdate( - projectId, - docId, - ranges, - appliedOps, - updatedDocLines, - function (error, newRanges, rangesWereCollapsed) { - UpdateManager._addProjectHistoryMetadataToOps( - appliedOps, - pathname, - projectHistoryId, - lines - ) - profile.log('RangesManager.applyUpdate', { sync: true }) - if (error) { - return callback(error) - } - RedisManager.updateDocument( - projectId, - docId, - updatedDocLines, - version, - appliedOps, - newRanges, - update.meta, - function (error, projectOpsLength) { - profile.log('RedisManager.updateDocument') - if (error) { - return callback(error) - } - HistoryManager.recordAndFlushHistoryOps( - projectId, - appliedOps, - projectOpsLength - ) - profile.log('recordAndFlushHistoryOps') - if (rangesWereCollapsed) { - Metrics.inc('doc-snapshot') - logger.debug( - { - projectId, - docId, - previousVersion, - lines, - ranges, - update, - }, - 'update collapsed some ranges, snapshotting previous content' - ) - // Do this last, since it's a mongo call, and so potentially longest running - // If it overruns the lock, it's ok, since all of our redis work is done - SnapshotManager.recordSnapshot( - projectId, - docId, - previousVersion, - pathname, - lines, - ranges, - function (error) { - if (error) { - logger.error( - { - err: error, - projectId, - docId, - version, - lines, - ranges, - }, - 'error recording snapshot' - ) - callback(error) - } else { - callback() - } - } - ) - } else { - callback() - } - } - ) - } - ) - } + version + )) + profile.log('sharejs.applyUpdate', { + // only synchronous when the update applies directly to the + // doc version, otherwise getPreviousDocOps is called. + sync: incomingUpdateVersion === previousVersion, + }) + + const { newRanges, rangesWereCollapsed } = + await RangesManager.promises.applyUpdate( + projectId, + docId, + ranges, + appliedOps, + updatedDocLines + ) + profile.log('RangesManager.applyUpdate', { sync: true }) + + UpdateManager._addProjectHistoryMetadataToOps( + appliedOps, + pathname, + projectHistoryId, + lines + ) + + const projectOpsLength = await RedisManager.promises.updateDocument( + projectId, + docId, + updatedDocLines, + version, + appliedOps, + newRanges, + update.meta + ) + profile.log('RedisManager.updateDocument') + + HistoryManager.recordAndFlushHistoryOps( + projectId, + appliedOps, + projectOpsLength + ) + profile.log('recordAndFlushHistoryOps') + + if (rangesWereCollapsed) { + Metrics.inc('doc-snapshot') + logger.debug( + { + projectId, + docId, + previousVersion, + lines, + ranges, + update, + }, + 'update collapsed some ranges, snapshotting previous content' + ) + + // Do this last, since it's a mongo call, and so potentially longest running + // If it overruns the lock, it's ok, since all of our redis work is done + await SnapshotManager.promises.recordSnapshot( + projectId, + docId, + previousVersion, + pathname, + lines, + ranges ) } - ) + } catch (error) { + RealTimeRedisManager.sendData({ + project_id: projectId, + doc_id: docId, + error: error instanceof Error ? error.message : error, + }) + profile.log('sendData') + throw error + } finally { + profile.end() + } }, - lockUpdatesAndDo(method, projectId, docId, ...rest) { - const adjustedLength = Math.max(rest.length, 1) - const args = rest.slice(0, adjustedLength - 1) - const callback = rest[adjustedLength - 1] + // lockUpdatesAndDo can't be promisified yet because it expects a + // callback-style function + async lockUpdatesAndDo(method, projectId, docId, ...args) { const profile = new Profiler('lockUpdatesAndDo', { project_id: projectId, doc_id: docId, }) - return LockManager.getLock(docId, function (error, lockValue) { - profile.log('getLock') - if (error) { - return callback(error) - } - UpdateManager.processOutstandingUpdates( - projectId, - docId, - function (error) { - if (error) { - return UpdateManager._handleErrorInsideLock( - docId, - lockValue, - error, - callback - ) - } - profile.log('processOutstandingUpdates') - method( - projectId, - docId, - ...Array.from(args), - function (error, ...responseArgs) { - if (error) { - return UpdateManager._handleErrorInsideLock( - docId, - lockValue, - error, - callback - ) - } - profile.log('method') - LockManager.releaseLock(docId, lockValue, function (error) { - if (error) { - return callback(error) - } - profile.log('releaseLock').end() - callback(null, ...Array.from(responseArgs)) - // We held the lock for a while so updates might have queued up - UpdateManager.continueProcessingUpdatesWithLock( - projectId, - docId, - err => { - if (err) { - // The processing may fail for invalid user updates. - // This can be very noisy, put them on level DEBUG - // and record a metric. - Metrics.inc('background-processing-updates-error') - logger.debug( - { err, projectId, docId }, - 'error processing updates in background' - ) - } - } - ) - }) - } - ) - } - ) - }) - }, - _handleErrorInsideLock(docId, lockValue, originalError, callback) { - if (!callback) { - callback = function () {} + const lockValue = await LockManager.promises.getLock(docId) + profile.log('getLock') + + let responseArgs + try { + await UpdateManager.processOutstandingUpdates(projectId, docId) + profile.log('processOutstandingUpdates') + + // TODO: method is still a callback-style function. Change this when promisifying DocumentManager + responseArgs = await new Promise((resolve, reject) => { + method(projectId, docId, ...args, (error, ...responseArgs) => { + if (error) { + reject(error) + } else { + resolve(responseArgs) + } + }) + }) + profile.log('method') + } finally { + await LockManager.promises.releaseLock(docId, lockValue) + profile.log('releaseLock').end() } - LockManager.releaseLock(docId, lockValue, lockError => - callback(originalError) + + // We held the lock for a while so updates might have queued up + UpdateManager.continueProcessingUpdatesWithLock(projectId, docId).catch( + err => { + // The processing may fail for invalid user updates. + // This can be very noisy, put them on level DEBUG + // and record a metric. + Metrics.inc('background-processing-updates-error') + logger.debug( + { err, projectId, docId }, + 'error processing updates in background' + ) + } ) + + return responseArgs }, _sanitizeUpdate(update) { @@ -372,7 +251,7 @@ module.exports = UpdateManager = { // 16-bit character of a blackboard bold character (http://www.fileformat.info/info/unicode/char/1d400/index.htm). // Something must be going on client side that is screwing up the encoding and splitting the // two 16-bit characters so that \uD835 is standalone. - for (const op of Array.from(update.op || [])) { + for (const op of update.op || []) { if (op.i != null) { // Replace high and low surrogate characters with 'replacement character' (\uFFFD) op.i = op.i.replace(/[\uD800-\uDFFF]/g, '\uFFFD') @@ -384,7 +263,7 @@ module.exports = UpdateManager = { _addProjectHistoryMetadataToOps(updates, pathname, projectHistoryId, lines) { let docLength = _.reduce(lines, (chars, line) => chars + line.length, 0) docLength += lines.length - 1 // count newline characters - return updates.forEach(function (update) { + updates.forEach(function (update) { update.projectHistoryId = projectHistoryId if (!update.meta) { update.meta = {} @@ -400,20 +279,41 @@ module.exports = UpdateManager = { // We want to include the doc_length at the start of each update, // before it's ops are applied. However, we need to track any // changes to it for the next update. - return (() => { - const result = [] - for (const op of Array.from(update.op)) { - if (op.i != null) { - docLength += op.i.length - } - if (op.d != null) { - result.push((docLength -= op.d.length)) - } else { - result.push(undefined) - } + for (const op of update.op) { + if (op.i != null) { + docLength += op.i.length } - return result - })() + if (op.d != null) { + docLength -= op.d.length + } + } }) }, } + +const CallbackifiedUpdateManager = callbackifyAll(UpdateManager) + +module.exports = CallbackifiedUpdateManager +module.exports.promises = UpdateManager + +module.exports.lockUpdatesAndDo = function lockUpdatesAndDo( + method, + projectId, + docId, + ...rest +) { + const adjustedLength = Math.max(rest.length, 1) + const args = rest.slice(0, adjustedLength - 1) + const callback = rest[adjustedLength - 1] + + // TODO: During the transition to promises, UpdateManager.lockUpdatesAndDo + // returns the potentially multiple arguments that must be provided to the + // callback in an array. + UpdateManager.lockUpdatesAndDo(method, projectId, docId, ...args) + .then(responseArgs => { + callback(null, ...responseArgs) + }) + .catch(err => { + callback(err) + }) +} diff --git a/services/document-updater/package.json b/services/document-updater/package.json index 612ab7460c..1a15b3172e 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -25,6 +25,7 @@ "@overleaf/ranges-tracker": "*", "@overleaf/redis-wrapper": "*", "@overleaf/settings": "*", + "@types/chai-as-promised": "^7.1.8", "async": "^3.2.2", "body-parser": "^1.19.0", "bunyan": "^1.8.15", diff --git a/services/document-updater/test/setup.js b/services/document-updater/test/setup.js index b0dfbc7028..5c858eee05 100644 --- a/services/document-updater/test/setup.js +++ b/services/document-updater/test/setup.js @@ -1,12 +1,15 @@ const chai = require('chai') +const chaiAsPromised = require('chai-as-promised') +const sinonChai = require('sinon-chai') const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') // Chai configuration chai.should() +chai.use(chaiAsPromised) // Load sinon-chai assertions so expect(stubFn).to.have.been.calledWith('abc') // has a nicer failure messages -chai.use(require('sinon-chai')) +chai.use(sinonChai) // Global stubs const sandbox = sinon.createSandbox() diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 249c840082..932dc472db 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -1,82 +1,113 @@ -/* eslint-disable - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS206: Consider reworking classes to avoid initClass - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ +// @ts-check + const sinon = require('sinon') -const modulePath = '../../../../app/js/UpdateManager.js' +const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') +const MODULE_PATH = '../../../../app/js/UpdateManager.js' + describe('UpdateManager', function () { beforeEach(function () { - let Profiler, Timer this.project_id = 'project-id-123' this.projectHistoryId = 'history-id-123' this.doc_id = 'document-id-123' - this.callback = sinon.stub() - this.UpdateManager = SandboxedModule.require(modulePath, { + this.lockValue = 'mock-lock-value' + + this.Metrics = { + inc: sinon.stub(), + Timer: class Timer {}, + } + this.Metrics.Timer.prototype.done = sinon.stub() + + this.Profiler = class Profiler {} + this.Profiler.prototype.log = sinon.stub().returns({ end: sinon.stub() }) + this.Profiler.prototype.end = sinon.stub() + + this.LockManager = { + promises: { + tryLock: sinon.stub().resolves(this.lockValue), + getLock: sinon.stub().resolves(this.lockValue), + releaseLock: sinon.stub().resolves(), + }, + } + + this.RedisManager = { + promises: { + setDocument: sinon.stub().resolves(), + updateDocument: sinon.stub(), + }, + } + + this.RealTimeRedisManager = { + sendData: sinon.stub(), + promises: { + getUpdatesLength: sinon.stub(), + getPendingUpdatesForDoc: sinon.stub(), + }, + } + + this.ShareJsUpdateManager = { + promises: { + applyUpdate: sinon.stub(), + }, + } + + this.HistoryManager = { + recordAndFlushHistoryOps: sinon.stub(), + } + + this.Settings = {} + + this.DocumentManager = { + promises: { + getDoc: sinon.stub(), + }, + } + + this.RangesManager = { + promises: { + applyUpdate: sinon.stub(), + }, + } + + this.SnapshotManager = { + promises: { + recordSnapshot: sinon.stub().resolves(), + }, + } + + this.UpdateManager = SandboxedModule.require(MODULE_PATH, { requires: { - './LockManager': (this.LockManager = {}), - './RedisManager': (this.RedisManager = {}), - './RealTimeRedisManager': (this.RealTimeRedisManager = {}), - './ShareJsUpdateManager': (this.ShareJsUpdateManager = {}), - './HistoryManager': (this.HistoryManager = {}), - './Metrics': (this.Metrics = { - inc: sinon.stub(), - Timer: (Timer = (function () { - Timer = class Timer { - static initClass() { - this.prototype.done = sinon.stub() - } - } - Timer.initClass() - return Timer - })()), - }), - '@overleaf/settings': (this.Settings = {}), - './DocumentManager': (this.DocumentManager = {}), - './RangesManager': (this.RangesManager = {}), - './SnapshotManager': (this.SnapshotManager = {}), - './Profiler': (Profiler = (function () { - Profiler = class Profiler { - static initClass() { - this.prototype.log = sinon.stub().returns({ end: sinon.stub() }) - this.prototype.end = sinon.stub() - } - } - Profiler.initClass() - return Profiler - })()), + './LockManager': this.LockManager, + './RedisManager': this.RedisManager, + './RealTimeRedisManager': this.RealTimeRedisManager, + './ShareJsUpdateManager': this.ShareJsUpdateManager, + './HistoryManager': this.HistoryManager, + './Metrics': this.Metrics, + '@overleaf/settings': this.Settings, + './DocumentManager': this.DocumentManager, + './RangesManager': this.RangesManager, + './SnapshotManager': this.SnapshotManager, + './Profiler': this.Profiler, }, }) }) describe('processOutstandingUpdates', function () { - beforeEach(function () { - this.UpdateManager.fetchAndApplyUpdates = sinon.stub().callsArg(2) - this.UpdateManager.processOutstandingUpdates( + beforeEach(async function () { + this.UpdateManager.promises.fetchAndApplyUpdates = sinon.stub().resolves() + await this.UpdateManager.promises.processOutstandingUpdates( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should apply the updates', function () { - this.UpdateManager.fetchAndApplyUpdates + this.UpdateManager.promises.fetchAndApplyUpdates .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - it('should call the callback', function () { - this.callback.called.should.equal(true) - }) - it('should time the execution', function () { this.Metrics.Timer.prototype.done.called.should.equal(true) }) @@ -85,219 +116,184 @@ describe('UpdateManager', function () { describe('processOutstandingUpdatesWithLock', function () { describe('when the lock is free', function () { beforeEach(function () { - this.LockManager.tryLock = sinon + this.UpdateManager.promises.continueProcessingUpdatesWithLock = sinon .stub() - .callsArgWith(1, null, true, (this.lockValue = 'mock-lock-value')) - this.LockManager.releaseLock = sinon.stub().callsArg(2) - this.UpdateManager.continueProcessingUpdatesWithLock = sinon + .resolves() + this.UpdateManager.promises.processOutstandingUpdates = sinon .stub() - .callsArg(2) - this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) + .resolves() }) describe('successfully', function () { - beforeEach(function () { - this.UpdateManager.processOutstandingUpdatesWithLock( + beforeEach(async function () { + await this.UpdateManager.promises.processOutstandingUpdatesWithLock( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should acquire the lock', function () { - this.LockManager.tryLock.calledWith(this.doc_id).should.equal(true) + this.LockManager.promises.tryLock + .calledWith(this.doc_id) + .should.equal(true) }) it('should free the lock', function () { - this.LockManager.releaseLock + this.LockManager.promises.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) it('should process the outstanding updates', function () { - this.UpdateManager.processOutstandingUpdates + this.UpdateManager.promises.processOutstandingUpdates .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should do everything with the lock acquired', function () { - this.UpdateManager.processOutstandingUpdates - .calledAfter(this.LockManager.tryLock) + this.UpdateManager.promises.processOutstandingUpdates + .calledAfter(this.LockManager.promises.tryLock) .should.equal(true) - this.UpdateManager.processOutstandingUpdates - .calledBefore(this.LockManager.releaseLock) + this.UpdateManager.promises.processOutstandingUpdates + .calledBefore(this.LockManager.promises.releaseLock) .should.equal(true) }) it('should continue processing new updates that may have come in', function () { - this.UpdateManager.continueProcessingUpdatesWithLock + this.UpdateManager.promises.continueProcessingUpdatesWithLock .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - - it('should return the callback', function () { - this.callback.called.should.equal(true) - }) }) describe('when processOutstandingUpdates returns an error', function () { - beforeEach(function () { - this.UpdateManager.processOutstandingUpdates = sinon + beforeEach(async function () { + this.error = new Error('Something went wrong') + this.UpdateManager.promises.processOutstandingUpdates = sinon .stub() - .callsArgWith(2, (this.error = new Error('Something went wrong'))) - this.UpdateManager.processOutstandingUpdatesWithLock( - this.project_id, - this.doc_id, - this.callback - ) + .rejects(this.error) + await expect( + this.UpdateManager.promises.processOutstandingUpdatesWithLock( + this.project_id, + this.doc_id + ) + ).to.be.rejectedWith(this.error) }) it('should free the lock', function () { - this.LockManager.releaseLock + this.LockManager.promises.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) - - it('should return the error in the callback', function () { - this.callback.calledWith(this.error).should.equal(true) - }) }) }) describe('when the lock is taken', function () { - beforeEach(function () { - this.LockManager.tryLock = sinon.stub().callsArgWith(1, null, false) - this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) - this.UpdateManager.processOutstandingUpdatesWithLock( + beforeEach(async function () { + this.LockManager.promises.tryLock.resolves(null) + this.UpdateManager.promises.processOutstandingUpdates = sinon + .stub() + .resolves() + await this.UpdateManager.promises.processOutstandingUpdatesWithLock( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) - it('should return the callback', function () { - this.callback.called.should.equal(true) - }) - it('should not process the updates', function () { - this.UpdateManager.processOutstandingUpdates.called.should.equal(false) + this.UpdateManager.promises.processOutstandingUpdates.called.should.equal( + false + ) }) }) }) describe('continueProcessingUpdatesWithLock', function () { describe('when there are outstanding updates', function () { - beforeEach(function () { - this.RealTimeRedisManager.getUpdatesLength = sinon + beforeEach(async function () { + this.RealTimeRedisManager.promises.getUpdatesLength.resolves(3) + this.UpdateManager.promises.processOutstandingUpdatesWithLock = sinon .stub() - .callsArgWith(1, null, 3) - this.UpdateManager.processOutstandingUpdatesWithLock = sinon - .stub() - .callsArg(2) - this.UpdateManager.continueProcessingUpdatesWithLock( + .resolves() + await this.UpdateManager.promises.continueProcessingUpdatesWithLock( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should process the outstanding updates', function () { - this.UpdateManager.processOutstandingUpdatesWithLock + this.UpdateManager.promises.processOutstandingUpdatesWithLock .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - - it('should return the callback', function () { - this.callback.called.should.equal(true) - }) }) describe('when there are no outstanding updates', function () { - beforeEach(function () { - this.RealTimeRedisManager.getUpdatesLength = sinon + beforeEach(async function () { + this.RealTimeRedisManager.promises.getUpdatesLength.resolves(0) + this.UpdateManager.promises.processOutstandingUpdatesWithLock = sinon .stub() - .callsArgWith(1, null, 0) - this.UpdateManager.processOutstandingUpdatesWithLock = sinon - .stub() - .callsArg(2) - this.UpdateManager.continueProcessingUpdatesWithLock( + .resolves() + await this.UpdateManager.promises.continueProcessingUpdatesWithLock( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should not try to process the outstanding updates', function () { - this.UpdateManager.processOutstandingUpdatesWithLock.called.should.equal( + this.UpdateManager.promises.processOutstandingUpdatesWithLock.called.should.equal( false ) }) - - it('should return the callback', function () { - this.callback.called.should.equal(true) - }) }) }) describe('fetchAndApplyUpdates', function () { describe('with updates', function () { - beforeEach(function () { + beforeEach(async function () { this.updates = [{ p: 1, t: 'foo' }] this.updatedDocLines = ['updated', 'lines'] this.version = 34 - this.RealTimeRedisManager.getPendingUpdatesForDoc = sinon - .stub() - .callsArgWith(1, null, this.updates) - this.UpdateManager.applyUpdate = sinon - .stub() - .callsArgWith(3, null, this.updatedDocLines, this.version) - this.UpdateManager.fetchAndApplyUpdates( + this.RealTimeRedisManager.promises.getPendingUpdatesForDoc.resolves( + this.updates + ) + this.UpdateManager.promises.applyUpdate = sinon.stub().resolves() + await this.UpdateManager.promises.fetchAndApplyUpdates( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should get the pending updates', function () { - this.RealTimeRedisManager.getPendingUpdatesForDoc + this.RealTimeRedisManager.promises.getPendingUpdatesForDoc .calledWith(this.doc_id) .should.equal(true) }) it('should apply the updates', function () { - Array.from(this.updates).map(update => - this.UpdateManager.applyUpdate + this.updates.map(update => + this.UpdateManager.promises.applyUpdate .calledWith(this.project_id, this.doc_id, update) .should.equal(true) ) }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) - }) }) describe('when there are no updates', function () { - beforeEach(function () { + beforeEach(async function () { this.updates = [] - this.RealTimeRedisManager.getPendingUpdatesForDoc = sinon - .stub() - .callsArgWith(1, null, this.updates) - this.UpdateManager.applyUpdate = sinon.stub() - this.RedisManager.setDocument = sinon.stub() - this.UpdateManager.fetchAndApplyUpdates( + this.RealTimeRedisManager.promises.getPendingUpdatesForDoc.resolves( + this.updates + ) + this.UpdateManager.promises.applyUpdate = sinon.stub().resolves() + await this.UpdateManager.promises.fetchAndApplyUpdates( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should not call applyUpdate', function () { - this.UpdateManager.applyUpdate.called.should.equal(false) - }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) + this.UpdateManager.promises.applyUpdate.called.should.equal(false) }) }) }) @@ -315,44 +311,41 @@ describe('UpdateManager', function () { { v: 42, op: 'mock-op-42' }, { v: 45, op: 'mock-op-45' }, ] - this.project_ops_length = sinon.stub() + this.project_ops_length = 123 this.pathname = '/a/b/c.tex' - this.DocumentManager.getDoc = sinon - .stub() - .yields( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) - this.RangesManager.applyUpdate = sinon - .stub() - .yields(null, this.updated_ranges, false) - this.ShareJsUpdateManager.applyUpdate = sinon - .stub() - .yields(null, this.updatedDocLines, this.version, this.appliedOps) - this.RedisManager.updateDocument = sinon - .stub() - .yields(null, this.project_ops_length) - this.RealTimeRedisManager.sendData = sinon.stub() - this.UpdateManager._addProjectHistoryMetadataToOps = sinon.stub() - this.HistoryManager.recordAndFlushHistoryOps = sinon.stub() + this.DocumentManager.promises.getDoc.resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + }) + this.RangesManager.promises.applyUpdate.resolves({ + newRanges: this.updated_ranges, + rangesWereCollapsed: false, + }) + this.ShareJsUpdateManager.promises.applyUpdate = sinon.stub().resolves({ + updatedDocLines: this.updatedDocLines, + version: this.version, + appliedOps: this.appliedOps, + }) + this.RedisManager.promises.updateDocument.resolves( + this.project_ops_length + ) + this.UpdateManager.promises._addProjectHistoryMetadataToOps = sinon.stub() }) describe('normally', function () { - beforeEach(function () { - this.UpdateManager.applyUpdate( + beforeEach(async function () { + await this.UpdateManager.promises.applyUpdate( this.project_id, this.doc_id, - this.update, - this.callback + this.update ) }) it('should apply the updates via ShareJS', function () { - this.ShareJsUpdateManager.applyUpdate + this.ShareJsUpdateManager.promises.applyUpdate .calledWith( this.project_id, this.doc_id, @@ -364,7 +357,7 @@ describe('UpdateManager', function () { }) it('should update the ranges', function () { - this.RangesManager.applyUpdate + this.RangesManager.promises.applyUpdate .calledWith( this.project_id, this.doc_id, @@ -376,7 +369,7 @@ describe('UpdateManager', function () { }) it('should save the document', function () { - this.RedisManager.updateDocument + this.RedisManager.promises.updateDocument .calledWith( this.project_id, this.doc_id, @@ -390,14 +383,12 @@ describe('UpdateManager', function () { }) it('should add metadata to the ops', function () { - this.UpdateManager._addProjectHistoryMetadataToOps - .calledWith( - this.appliedOps, - this.pathname, - this.projectHistoryId, - this.lines - ) - .should.equal(true) + this.UpdateManager.promises._addProjectHistoryMetadataToOps.should.have.been.calledWith( + this.appliedOps, + this.pathname, + this.projectHistoryId, + this.lines + ) }) it('should push the applied ops into the history queue', function () { @@ -405,25 +396,20 @@ describe('UpdateManager', function () { .calledWith(this.project_id, this.appliedOps, this.project_ops_length) .should.equal(true) }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) - }) }) describe('with UTF-16 surrogate pairs in the update', function () { - beforeEach(function () { + beforeEach(async function () { this.update = { op: [{ p: 42, i: '\uD835\uDC00' }] } - this.UpdateManager.applyUpdate( + await this.UpdateManager.promises.applyUpdate( this.project_id, this.doc_id, - this.update, - this.callback + this.update ) }) it('should apply the update but with surrogate pairs removed', function () { - this.ShareJsUpdateManager.applyUpdate + this.ShareJsUpdateManager.promises.applyUpdate .calledWith(this.project_id, this.doc_id, this.update) .should.equal(true) @@ -433,15 +419,16 @@ describe('UpdateManager', function () { }) describe('with an error', function () { - beforeEach(function () { + beforeEach(async function () { this.error = new Error('something went wrong') - this.ShareJsUpdateManager.applyUpdate = sinon.stub().yields(this.error) - this.UpdateManager.applyUpdate( - this.project_id, - this.doc_id, - this.update, - this.callback - ) + this.ShareJsUpdateManager.promises.applyUpdate.rejects(this.error) + await expect( + this.UpdateManager.promises.applyUpdate( + this.project_id, + this.doc_id, + this.update + ) + ).to.be.rejectedWith(this.error) }) it('should call RealTimeRedisManager.sendData with the error', function () { @@ -453,23 +440,18 @@ describe('UpdateManager', function () { }) .should.equal(true) }) - - it('should call the callback with the error', function () { - this.callback.calledWith(this.error).should.equal(true) - }) }) describe('when ranges get collapsed', function () { - beforeEach(function () { - this.RangesManager.applyUpdate = sinon - .stub() - .yields(null, this.updated_ranges, true) - this.SnapshotManager.recordSnapshot = sinon.stub().yields() - this.UpdateManager.applyUpdate( + beforeEach(async function () { + this.RangesManager.promises.applyUpdate.resolves({ + newRanges: this.updated_ranges, + rangesWereCollapsed: true, + }) + await this.UpdateManager.promises.applyUpdate( this.project_id, this.doc_id, - this.update, - this.callback + this.update ) }) @@ -478,7 +460,7 @@ describe('UpdateManager', function () { }) it('should call SnapshotManager.recordSnapshot', function () { - this.SnapshotManager.recordSnapshot + this.SnapshotManager.promises.recordSnapshot .calledWith( this.project_id, this.doc_id, @@ -558,38 +540,41 @@ describe('UpdateManager', function () { describe('lockUpdatesAndDo', function () { beforeEach(function () { - this.method = sinon.stub().callsArgWith(3, null, this.response_arg1) - this.callback = sinon.stub() + this.method = sinon + .stub() + .yields(null, this.response_arg1, this.response_arg2) this.arg1 = 'argument 1' this.response_arg1 = 'response argument 1' - this.lockValue = 'mock-lock-value' - this.LockManager.getLock = sinon - .stub() - .callsArgWith(1, null, this.lockValue) - this.LockManager.releaseLock = sinon.stub().callsArg(2) + this.response_arg2 = 'response argument 2' }) describe('successfully', function () { - beforeEach(function () { - this.UpdateManager.continueProcessingUpdatesWithLock = sinon.stub() - this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) - this.UpdateManager.lockUpdatesAndDo( + beforeEach(async function () { + this.UpdateManager.promises.continueProcessingUpdatesWithLock = sinon + .stub() + .resolves() + this.UpdateManager.promises.processOutstandingUpdates = sinon + .stub() + .resolves() + this.response = await this.UpdateManager.promises.lockUpdatesAndDo( this.method, this.project_id, this.doc_id, - this.arg1, - this.callback + this.arg1 ) }) it('should lock the doc', function () { - this.LockManager.getLock.calledWith(this.doc_id).should.equal(true) + this.LockManager.promises.getLock + .calledWith(this.doc_id) + .should.equal(true) }) it('should process any outstanding updates', function () { - this.UpdateManager.processOutstandingUpdates - .calledWith(this.project_id, this.doc_id) - .should.equal(true) + this.UpdateManager.promises.processOutstandingUpdates.should.have.been.calledWith( + this.project_id, + this.doc_id + ) }) it('should call the method', function () { @@ -598,76 +583,71 @@ describe('UpdateManager', function () { .should.equal(true) }) - it('should return the method response to the callback', function () { - this.callback.calledWith(null, this.response_arg1).should.equal(true) + it('should return the method response arguments', function () { + expect(this.response).to.deep.equal([ + this.response_arg1, + this.response_arg2, + ]) }) it('should release the lock', function () { - this.LockManager.releaseLock + this.LockManager.promises.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) it('should continue processing updates', function () { - this.UpdateManager.continueProcessingUpdatesWithLock + this.UpdateManager.promises.continueProcessingUpdatesWithLock .calledWith(this.project_id, this.doc_id) .should.equal(true) }) }) describe('when processOutstandingUpdates returns an error', function () { - beforeEach(function () { - this.UpdateManager.processOutstandingUpdates = sinon + beforeEach(async function () { + this.error = new Error('Something went wrong') + this.UpdateManager.promises.processOutstandingUpdates = sinon .stub() - .callsArgWith(2, (this.error = new Error('Something went wrong'))) - this.UpdateManager.lockUpdatesAndDo( - this.method, - this.project_id, - this.doc_id, - this.arg1, - this.callback - ) + .rejects(this.error) + await expect( + this.UpdateManager.promises.lockUpdatesAndDo( + this.method, + this.project_id, + this.doc_id, + this.arg1 + ) + ).to.be.rejectedWith(this.error) }) it('should free the lock', function () { - this.LockManager.releaseLock + this.LockManager.promises.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) - - it('should return the error in the callback', function () { - this.callback.calledWith(this.error).should.equal(true) - }) }) describe('when the method returns an error', function () { - beforeEach(function () { - this.UpdateManager.processOutstandingUpdates = sinon.stub().callsArg(2) - this.method = sinon + beforeEach(async function () { + this.error = new Error('something went wrong') + this.UpdateManager.promises.processOutstandingUpdates = sinon .stub() - .callsArgWith( - 3, - (this.error = new Error('something went wrong')), - this.response_arg1 + .resolves() + this.method = sinon.stub().yields(this.error) + await expect( + this.UpdateManager.promises.lockUpdatesAndDo( + this.method, + this.project_id, + this.doc_id, + this.arg1 ) - this.UpdateManager.lockUpdatesAndDo( - this.method, - this.project_id, - this.doc_id, - this.arg1, - this.callback - ) + ).to.be.rejectedWith(this.error) }) it('should free the lock', function () { - this.LockManager.releaseLock + this.LockManager.promises.releaseLock .calledWith(this.doc_id, this.lockValue) .should.equal(true) }) - - it('should return the error in the callback', function () { - this.callback.calledWith(this.error).should.equal(true) - }) }) }) })