diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index 4160309553..3f4b03b68c 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -199,7 +199,11 @@ const RETRYABLE_ERRORS = new Set([ function isRetryable(error) { // use the same retryable errors as requestretry - if (error instanceof Errors.WebApiServerError) { + // node-fetch uses AbortError: + // https://github.com/node-fetch/node-fetch/blob/main/docs/ERROR-HANDLING.md + if (error.name === 'AbortError') { + return true + } else if (error instanceof Errors.WebApiServerError) { const status = error.info?.status return ( typeof status === 'number' && diff --git a/services/document-updater/test/acceptance/js/GettingADocumentTests.js b/services/document-updater/test/acceptance/js/GettingADocumentTests.js index 6fda581ab7..9ec3ee5c8d 100644 --- a/services/document-updater/test/acceptance/js/GettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/GettingADocumentTests.js @@ -338,12 +338,48 @@ describe('Getting a document', function () { }) }) - describe('when the web api http request takes a long time', function () { + describe('when the web api http request times out on the first request', function () { + before(function (done) { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + }) + sinon + .stub(MockWebApi, 'getDocument') + .onFirstCall() + .returns( + new Promise(resolve => { + setTimeout(() => resolve(null), 30_000) + }) + ) + .callThrough() // subsequent requests return normally + done() + }) + + after(function () { + MockWebApi.getDocument.restore() + }) + + it('should retry the request and return the document', async function () { + const returnedDoc = await DocUpdaterClient.getDoc( + this.project_id, + this.doc_id + ) + expect(returnedDoc).to.deep.include({ + lines: this.lines, + version: this.version, + }) + }) + }) + + describe('when the web api http request times out repeatedly', function () { before(function (done) { this.timeout = 10000 sinon.stub(MockWebApi, 'getDocument').returns( new Promise(resolve => { - setTimeout(() => resolve(null), 30000) + setTimeout(() => resolve(null), 30_000) }) ) done() @@ -353,7 +389,7 @@ describe('Getting a document', function () { MockWebApi.getDocument.restore() }) - it('should return quickly(ish)', async function () { + it('should return an error after two attempts', async function () { const projectId = DocUpdaterClient.randomId() const docId = DocUpdaterClient.randomId() const start = Date.now() @@ -361,7 +397,8 @@ describe('Getting a document', function () { .to.be.rejectedWith(RequestFailedError) .and.eventually.have.nested.property('response.status', 500) const delta = Date.now() - start - expect(delta).to.be.below(20000) + expect(delta).to.be.above(10_000) + expect(delta).to.be.below(20_000) }) }) }) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index f6f28b4ceb..961cc7c44e 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -916,4 +916,89 @@ describe('Setting a document', function () { expect(MockWebApi.setDocumentController).to.be.calledTwice }) }) + + describe('when the web api http request times out on the first request', function () { + beforeEach(async function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + projectHistoryId: this.project_id, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + + const origSetDocumentController = + MockWebApi.setDocumentController.bind(MockWebApi) + const setDocumentStub = sinon + .stub(MockWebApi, 'setDocumentController') + .onFirstCall() + .callsFake(async (req, res, next) => { + await setTimeout(30_000) + }) + + setDocumentStub.onCall(1).callsFake(origSetDocumentController) + }) + + afterEach(function () { + MockWebApi.setDocumentController.restore() + }) + + it('should retry the request and return the document', async function () { + this.timeout(10000) + const returnedDoc = await DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false + ) + expect(returnedDoc).to.deep.include({ rev: '123' }) + expect(MockWebApi.setDocumentController).to.be.calledTwice + }) + }) + + describe('when the web api http request times out repeatedly', function () { + beforeEach(async function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + projectHistoryId: this.project_id, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + + sinon + .stub(MockWebApi, 'setDocumentController') + .callsFake(async (req, res, next) => { + await setTimeout(30_000) + }) + }) + + afterEach(function () { + MockWebApi.setDocumentController.restore() + }) + + it('should return an error after two attempts', async function () { + this.timeout(15000) + const start = Date.now() + await expect( + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false + ) + ).to.be.rejectedWith('request failed') + + const delta = Date.now() - start + expect(delta).to.be.above(10_000) // 2 * 5000ms timeout + expect(delta).to.be.below(20_000) + expect(MockWebApi.setDocumentController).to.be.calledTwice + }) + }) })