From 718da08e3bfa4c5854a76c860e85358b35242060 Mon Sep 17 00:00:00 2001 From: Antonio Davi Macedo Coelho de Castro Date: Fri, 12 Feb 2021 15:47:56 -0800 Subject: [PATCH 1/4] Fix flaky test with transactions --- spec/ParseServerRESTController.spec.js | 7 +- .../Storage/Mongo/MongoStorageAdapter.js | 17 +++- src/ParseServerRESTController.js | 86 +++++++++------- src/batch.js | 97 +++++++++++-------- 4 files changed, 126 insertions(+), 81 deletions(-) diff --git a/spec/ParseServerRESTController.spec.js b/spec/ParseServerRESTController.spec.js index fcf8281aff..1569880e62 100644 --- a/spec/ParseServerRESTController.spec.js +++ b/spec/ParseServerRESTController.spec.js @@ -197,7 +197,7 @@ describe('ParseServerRESTController', () => { .then(() => { spyOn(databaseAdapter, 'createObject').and.callThrough(); - RESTController.request('POST', 'batch', { + return RESTController.request('POST', 'batch', { requests: [ { method: 'POST', @@ -218,7 +218,7 @@ describe('ParseServerRESTController', () => { expect(response[1].success.objectId).toBeDefined(); expect(response[1].success.createdAt).toBeDefined(); const query = new Parse.Query('MyObject'); - query.find().then(results => { + return query.find().then(results => { expect(databaseAdapter.createObject.calls.count()).toBe(2); expect(databaseAdapter.createObject.calls.argsFor(0)[3]).toBe( databaseAdapter.createObject.calls.argsFor(1)[3] @@ -230,7 +230,8 @@ describe('ParseServerRESTController', () => { done(); }); }); - }); + }) + .catch(done.fail); }); it('should not save anything when one operation fails in a transaction', done => { diff --git a/src/Adapters/Storage/Mongo/MongoStorageAdapter.js b/src/Adapters/Storage/Mongo/MongoStorageAdapter.js index cac2f6614c..94c2ca4039 100644 --- a/src/Adapters/Storage/Mongo/MongoStorageAdapter.js +++ b/src/Adapters/Storage/Mongo/MongoStorageAdapter.js @@ -1046,9 +1046,20 @@ export class MongoStorageAdapter implements StorageAdapter { } commitTransactionalSession(transactionalSection: any): Promise { - return transactionalSection.commitTransaction().then(() => { - transactionalSection.endSession(); - }); + const commit = retries => { + return transactionalSection + .commitTransaction() + .catch(error => { + if (error && error.hasErrorLabel('TransientTransactionError') && retries > 0) { + return commit(retries - 1); + } + throw error; + }) + .then(() => { + transactionalSection.endSession(); + }); + }; + return commit(5); } abortTransactionalSession(transactionalSection: any): Promise { diff --git a/src/ParseServerRESTController.js b/src/ParseServerRESTController.js index c678f99ea3..8293b94a85 100644 --- a/src/ParseServerRESTController.js +++ b/src/ParseServerRESTController.js @@ -48,44 +48,60 @@ function ParseServerRESTController(applicationId, router) { } if (path === '/batch') { - let initialPromise = Promise.resolve(); - if (data.transaction === true) { - initialPromise = config.database.createTransactionalSession(); - } - return initialPromise.then(() => { - const promises = data.requests.map(request => { - return handleRequest(request.method, request.path, request.body, options, config).then( - response => { - if (options.returnStatus) { - const status = response._status; - delete response._status; - return { success: response, _status: status }; + const batch = transactionRetries => { + let initialPromise = Promise.resolve(); + if (data.transaction === true) { + initialPromise = config.database.createTransactionalSession(); + } + return initialPromise.then(() => { + const promises = data.requests.map(request => { + return handleRequest(request.method, request.path, request.body, options, config).then( + response => { + if (options.returnStatus) { + const status = response._status; + delete response._status; + return { success: response, _status: status }; + } + return { success: response }; + }, + error => { + return { + error: { code: error.code, error: error.message }, + }; } - return { success: response }; - }, - error => { - return { - error: { code: error.code, error: error.message }, - }; - } - ); - }); - return Promise.all(promises).then(result => { - if (data.transaction === true) { - if (result.find(resultItem => typeof resultItem.error === 'object')) { - return config.database.abortTransactionalSession().then(() => { - return Promise.reject(result); - }); - } else { - return config.database.commitTransactionalSession().then(() => { + ); + }); + return Promise.all(promises) + .then(result => { + if (data.transaction === true) { + if (result.find(resultItem => typeof resultItem.error === 'object')) { + return config.database.abortTransactionalSession().then(() => { + return Promise.reject(result); + }); + } else { + return config.database.commitTransactionalSession().then(() => { + return result; + }); + } + } else { return result; - }); - } - } else { - return result; - } + } + }) + .catch(error => { + if ( + error && + error.find( + errorItem => typeof errorItem.error === 'object' && errorItem.error.code === 251 + ) && + transactionRetries > 0 + ) { + return batch(transactionRetries - 1); + } + throw error; + }); }); - }); + }; + return batch(5); } let query; diff --git a/src/batch.js b/src/batch.js index 607c668a76..58c23ccab6 100644 --- a/src/batch.js +++ b/src/batch.js @@ -83,49 +83,66 @@ function handleBatch(router, req) { req.config.publicServerURL ); - let initialPromise = Promise.resolve(); - if (req.body.transaction === true) { - initialPromise = req.config.database.createTransactionalSession(); - } - - return initialPromise.then(() => { - const promises = req.body.requests.map(restRequest => { - const routablePath = makeRoutablePath(restRequest.path); - - // Construct a request that we can send to a handler - const request = { - body: restRequest.body, - config: req.config, - auth: req.auth, - info: req.info, - }; - - return router.tryRouteRequest(restRequest.method, routablePath, request).then( - response => { - return { success: response.response }; - }, - error => { - return { error: { code: error.code, error: error.message } }; - } - ); - }); + const batch = transactionRetries => { + let initialPromise = Promise.resolve(); + if (req.body.transaction === true) { + initialPromise = req.config.database.createTransactionalSession(); + } - return Promise.all(promises).then(results => { - if (req.body.transaction === true) { - if (results.find(result => typeof result.error === 'object')) { - return req.config.database.abortTransactionalSession().then(() => { - return Promise.reject({ response: results }); - }); - } else { - return req.config.database.commitTransactionalSession().then(() => { + return initialPromise.then(() => { + const promises = req.body.requests.map(restRequest => { + const routablePath = makeRoutablePath(restRequest.path); + + // Construct a request that we can send to a handler + const request = { + body: restRequest.body, + config: req.config, + auth: req.auth, + info: req.info, + }; + + return router.tryRouteRequest(restRequest.method, routablePath, request).then( + response => { + return { success: response.response }; + }, + error => { + return { error: { code: error.code, error: error.message } }; + } + ); + }); + + return Promise.all(promises) + .then(results => { + if (req.body.transaction === true) { + if (results.find(result => typeof result.error === 'object')) { + return req.config.database.abortTransactionalSession().then(() => { + return Promise.reject({ response: results }); + }); + } else { + return req.config.database.commitTransactionalSession().then(() => { + return { response: results }; + }); + } + } else { return { response: results }; - }); - } - } else { - return { response: results }; - } + } + }) + .catch(error => { + if ( + error && + error.response && + error.response.find( + errorItem => typeof errorItem.error === 'object' && errorItem.error.code === 251 + ) && + transactionRetries > 0 + ) { + return batch(transactionRetries - 1); + } + throw error; + }); }); - }); + }; + return batch(5); } module.exports = { From a32e997a264fb15b4d51784624a87e68f271a57b Mon Sep 17 00:00:00 2001 From: Antonio Davi Macedo Coelho de Castro Date: Fri, 12 Feb 2021 15:59:23 -0800 Subject: [PATCH 2/4] Add CHANGELOG entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1608343a46..e8f17d62e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ __BREAKING CHANGES:__ - NEW: Added file upload restriction. File upload is now only allowed for authenticated users by default for improved security. To allow file upload also for Anonymous Users or Public, set the `fileUpload` parameter in the [Parse Server Options](https://parseplatform.org/parse-server/api/master/ParseServerOptions.html). [#7071](https://github.com/parse-community/parse-server/pull/7071). Thanks to [dblythy](https://github.com/dblythy), [Manuel Trezza](https://github.com/mtrezza). ___ +- IMPROVE: Retry transactions on MongoDB when it fails due to transient error [#7187](https://github.com/parse-community/parse-server/pull/7187). Thanks to [Antonio Davi Macedo Coelho de Castro](https://github.com/davimacedo). - UPGRADE: Bump tests to use Mongo 4.4.4 [#7184](https://github.com/parse-community/parse-server/pull/7184). Thanks to [Antonio Davi Macedo Coelho de Castro](https://github.com/davimacedo). - NEW (EXPERIMENTAL): Added new page router with placeholder rendering and localization of custom and feature pages such as password reset and email verification. **Caution, this is an experimental feature that may not be appropriate for production.** [#6891](https://github.com/parse-community/parse-server/issues/6891). Thanks to [Manuel Trezza](https://github.com/mtrezza). - NEW: Added convenience method `Parse.Cloud.sendEmail(...)` to send email via email adapter in Cloud Code. [#7089](https://github.com/parse-community/parse-server/pull/7089). Thanks to [dblythy](https://github.com/dblythy) From bbf77696fbee36fbeff55185f44dd150352ba530 Mon Sep 17 00:00:00 2001 From: Antonio Davi Macedo Coelho de Castro Date: Mon, 15 Feb 2021 21:03:19 -0800 Subject: [PATCH 3/4] Fix the other transactions related tests that became flaky because now Parse Server tries to submit the transaction multilpe times in the case of TransientError --- spec/ParseServerRESTController.spec.js | 30 +++++++++++++++----------- spec/batch.spec.js | 28 ++++++++++++++---------- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/spec/ParseServerRESTController.spec.js b/spec/ParseServerRESTController.spec.js index 1569880e62..deb4d9afb0 100644 --- a/spec/ParseServerRESTController.spec.js +++ b/spec/ParseServerRESTController.spec.js @@ -219,10 +219,13 @@ describe('ParseServerRESTController', () => { expect(response[1].success.createdAt).toBeDefined(); const query = new Parse.Query('MyObject'); return query.find().then(results => { - expect(databaseAdapter.createObject.calls.count()).toBe(2); - expect(databaseAdapter.createObject.calls.argsFor(0)[3]).toBe( - databaseAdapter.createObject.calls.argsFor(1)[3] - ); + expect(databaseAdapter.createObject.calls.count() % 2).toBe(0); + expect(databaseAdapter.createObject.calls.count() > 0).toEqual(true); + for (let i = 0; i + 1 < databaseAdapter.createObject.calls.length; i = i + 2) { + expect(databaseAdapter.createObject.calls.argsFor(i)[3]).toBe( + databaseAdapter.createObject.calls.argsFor(i + 1)[3] + ); + } expect(results.map(result => result.get('key')).sort()).toEqual([ 'value1', 'value2', @@ -347,7 +350,7 @@ describe('ParseServerRESTController', () => { }); }); - it('should generate separate session for each call', async () => { + fit('should generate separate session for each call', async () => { const myObject = new Parse.Object('MyObject'); // This is important because transaction only works on pre-existing collections await myObject.save(); await myObject.destroy(); @@ -514,18 +517,18 @@ describe('ParseServerRESTController', () => { const results3 = await query3.find(); expect(results3.map(result => result.get('key')).sort()).toEqual(['value1', 'value2']); - expect(databaseAdapter.createObject.calls.count()).toBe(13); + expect(databaseAdapter.createObject.calls.count() >= 13).toEqual(true); let transactionalSession; let transactionalSession2; let myObjectDBCalls = 0; let myObject2DBCalls = 0; let myObject3DBCalls = 0; - for (let i = 0; i < 13; i++) { + for (let i = 0; i < databaseAdapter.createObject.calls.count(); i++) { const args = databaseAdapter.createObject.calls.argsFor(i); switch (args[0]) { case 'MyObject': myObjectDBCalls++; - if (!transactionalSession) { + if (!transactionalSession || (myObjectDBCalls - 1) % 2 === 0) { transactionalSession = args[3]; } else { expect(transactionalSession).toBe(args[3]); @@ -536,7 +539,7 @@ describe('ParseServerRESTController', () => { break; case 'MyObject2': myObject2DBCalls++; - if (!transactionalSession2) { + if (!transactionalSession2 || (myObject2DBCalls - 1) % 9 === 0) { transactionalSession2 = args[3]; } else { expect(transactionalSession2).toBe(args[3]); @@ -551,9 +554,12 @@ describe('ParseServerRESTController', () => { break; } } - expect(myObjectDBCalls).toEqual(2); - expect(myObject2DBCalls).toEqual(9); - expect(myObject3DBCalls).toEqual(2); + expect(myObjectDBCalls % 2).toEqual(0); + expect(myObjectDBCalls > 0).toEqual(true); + expect(myObject2DBCalls % 9).toEqual(0); + expect(myObject2DBCalls > 0).toEqual(true); + expect(myObject3DBCalls % 2).toEqual(0); + expect(myObject3DBCalls > 0).toEqual(true); }); }); } diff --git a/spec/batch.spec.js b/spec/batch.spec.js index db284d0ce4..07dc955664 100644 --- a/spec/batch.spec.js +++ b/spec/batch.spec.js @@ -228,10 +228,13 @@ describe('batch', () => { expect(response.data[1].success.createdAt).toBeDefined(); const query = new Parse.Query('MyObject'); query.find().then(results => { - expect(databaseAdapter.createObject.calls.count()).toBe(2); - expect(databaseAdapter.createObject.calls.argsFor(0)[3]).toBe( - databaseAdapter.createObject.calls.argsFor(1)[3] - ); + expect(databaseAdapter.createObject.calls.count() % 2).toBe(0); + expect(databaseAdapter.createObject.calls.count() > 0).toEqual(true); + for (let i = 0; i + 1 < databaseAdapter.createObject.calls.length; i = i + 2) { + expect(databaseAdapter.createObject.calls.argsFor(i)[3]).toBe( + databaseAdapter.createObject.calls.argsFor(i + 1)[3] + ); + } expect(results.map(result => result.get('key')).sort()).toEqual([ 'value1', 'value2', @@ -542,18 +545,18 @@ describe('batch', () => { const results3 = await query3.find(); expect(results3.map(result => result.get('key')).sort()).toEqual(['value1', 'value2']); - expect(databaseAdapter.createObject.calls.count()).toBe(13); + expect(databaseAdapter.createObject.calls.count() >= 13).toEqual(true); let transactionalSession; let transactionalSession2; let myObjectDBCalls = 0; let myObject2DBCalls = 0; let myObject3DBCalls = 0; - for (let i = 0; i < 13; i++) { + for (let i = 0; i < databaseAdapter.createObject.calls.count(); i++) { const args = databaseAdapter.createObject.calls.argsFor(i); switch (args[0]) { case 'MyObject': myObjectDBCalls++; - if (!transactionalSession) { + if (!transactionalSession || (myObjectDBCalls - 1) % 2 === 0) { transactionalSession = args[3]; } else { expect(transactionalSession).toBe(args[3]); @@ -564,7 +567,7 @@ describe('batch', () => { break; case 'MyObject2': myObject2DBCalls++; - if (!transactionalSession2) { + if (!transactionalSession2 || (myObject2DBCalls - 1) % 9 === 0) { transactionalSession2 = args[3]; } else { expect(transactionalSession2).toBe(args[3]); @@ -579,9 +582,12 @@ describe('batch', () => { break; } } - expect(myObjectDBCalls).toEqual(2); - expect(myObject2DBCalls).toEqual(9); - expect(myObject3DBCalls).toEqual(2); + expect(myObjectDBCalls % 2).toEqual(0); + expect(myObjectDBCalls > 0).toEqual(true); + expect(myObject2DBCalls % 9).toEqual(0); + expect(myObject2DBCalls > 0).toEqual(true); + expect(myObject3DBCalls % 2).toEqual(0); + expect(myObject3DBCalls > 0).toEqual(true); }); }); } From aef71fef6e6e0ca75ded5d88ba62d6d25ba8c201 Mon Sep 17 00:00:00 2001 From: Antonio Davi Macedo Coelho de Castro Date: Mon, 15 Feb 2021 21:08:29 -0800 Subject: [PATCH 4/4] Remove fit from tests --- spec/ParseServerRESTController.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/ParseServerRESTController.spec.js b/spec/ParseServerRESTController.spec.js index deb4d9afb0..e2f49c0bbe 100644 --- a/spec/ParseServerRESTController.spec.js +++ b/spec/ParseServerRESTController.spec.js @@ -350,7 +350,7 @@ describe('ParseServerRESTController', () => { }); }); - fit('should generate separate session for each call', async () => { + it('should generate separate session for each call', async () => { const myObject = new Parse.Object('MyObject'); // This is important because transaction only works on pre-existing collections await myObject.save(); await myObject.destroy();