From 2b7a1df809fd12233600a53c8e119835c35181d1 Mon Sep 17 00:00:00 2001 From: denihs Date: Fri, 15 Dec 2023 17:24:17 -0400 Subject: [PATCH] - fix some accounts tests - bring zodern's solution to async stubs --- packages/accounts-base/accounts_tests.js | 32 +-- packages/accounts-password/password_tests.js | 10 +- packages/ddp-client/client/client.js | 1 + .../ddp-client/client/queueStubsHelpers.js | 216 ++++++++++++++++++ .../ddp-client/common/livedata_connection.js | 66 +----- packages/ddp-client/test/livedata_tests.js | 12 +- 6 files changed, 246 insertions(+), 91 deletions(-) create mode 100644 packages/ddp-client/client/queueStubsHelpers.js diff --git a/packages/accounts-base/accounts_tests.js b/packages/accounts-base/accounts_tests.js index 506de9abe0..d6340dc076 100644 --- a/packages/accounts-base/accounts_tests.js +++ b/packages/accounts-base/accounts_tests.js @@ -238,7 +238,7 @@ Tinytest.addAsync('accounts - login token', async (test) => { connection = DDP.connect(Meteor.absoluteUrl()); // evil plan foiled await test.throwsAsync( - async () => await connection.callAsync('login', { returnStubValue: true }, { resume: stolenToken2 }), + async () => await connection.callAsync('login', { resume: stolenToken2 }), /You\'ve been logged out by the server/ ); connection.disconnect(); @@ -251,7 +251,7 @@ Tinytest.addAsync('accounts - login token', async (test) => { const stampedToken2 = Accounts._generateStampedLoginToken(); await insertUnhashedLoginToken(userId4, stampedToken2); connection = DDP.connect(Meteor.absoluteUrl()); - await connection.callAsync('login', { returnStubValue: true }, { resume: stampedToken2.token }); + await connection.callAsync('login', { resume: stampedToken2.token }); connection.disconnect(); // The token is no longer available to be stolen. @@ -296,15 +296,15 @@ Tinytest.addAsync('accounts - get new token', async test => { await Accounts._insertLoginToken(userId, stampedToken); const conn = DDP.connect(Meteor.absoluteUrl()); - await conn.callAsync('login', { returnStubValue: true }, { resume: stampedToken.token }); - test.equal(await conn.callAsync('getCurrentLoginToken', { returnServerPromise: true }), + await conn.callAsync('login', { resume: stampedToken.token }); + test.equal(await conn.callAsync('getCurrentLoginToken'), Accounts._hashLoginToken(stampedToken.token)); - const newTokenResult = await conn.callAsync('getNewToken', { returnServerPromise: true }); + const newTokenResult = await conn.callAsync('getNewToken'); test.equal(newTokenResult.tokenExpires, Accounts._tokenExpiration(stampedToken.when)); - const token = await conn.callAsync('getCurrentLoginToken', { returnServerPromise: true }); - test.equal(await conn.callAsync('getCurrentLoginToken', { returnServerPromise: true }), + const token = await conn.callAsync('getCurrentLoginToken'); + test.equal(await conn.callAsync('getCurrentLoginToken'), Accounts._hashLoginToken(newTokenResult.token)); conn.disconnect(); @@ -329,7 +329,7 @@ Tinytest.addAsync('accounts - remove other tokens', async (test) => { await Accounts._insertLoginToken(userId, stampedTokens[i]); const conn = DDP.connect(Meteor.absoluteUrl()); await conn.callAsync('login', { resume: stampedTokens[i].token }); - test.equal(await conn.callAsync('getCurrentLoginToken', { returnServerPromise: true }), + test.equal(await conn.callAsync('getCurrentLoginToken'), Accounts._hashLoginToken(stampedTokens[i].token)); conns.push(conn); } @@ -339,7 +339,7 @@ Tinytest.addAsync('accounts - remove other tokens', async (test) => { simplePoll(async () => { let tokens = []; for (const conn of conns) { - tokens.push(await conn.callAsync('getCurrentLoginToken', { returnServerPromise: true })); + tokens.push(await conn.callAsync('getCurrentLoginToken')); } return !tokens[1] && tokens[0] === Accounts._hashLoginToken(stampedTokens[0].token); @@ -381,18 +381,18 @@ Tinytest.addAsync( // On a new connection, Meteor.userId() should be null until logged in. let validateAttemptExpectedUserId = null; const onLoginExpectedUserId = userId; - await conn.callAsync('login', { returnServerPromise: true }, { resume: stampedToken.token }); + await conn.callAsync('login', { resume: stampedToken.token }); // Now that the user is logged in on the connection, Meteor.userId() should // return that user. validateAttemptExpectedUserId = userId; - await conn.callAsync('login', { returnServerPromise: true }, { resume: stampedToken.token }); + await conn.callAsync('login', { resume: stampedToken.token }); // Trigger onLoginFailure callbacks const onLoginFailureExpectedUserId = userId; await test.throwsAsync( async () => - await conn.callAsync('login', { returnServerPromise: true }, { resume: "bogus" }), '403'); + await conn.callAsync('login', { resume: "bogus" }), '403'); // Trigger onLogout callbacks const onLogoutExpectedUserId = userId; @@ -437,23 +437,23 @@ Tinytest.addAsync( // test a new connection let allowLogin = true; - await conn.callAsync('login', { returnServerPromise: true }, { resume: stampedToken.token }); + await conn.callAsync('login', { resume: stampedToken.token }); // Now that the user is logged in on the connection, Meteor.userId() should // return that user. - await conn.callAsync('login', { returnServerPromise: true }, { resume: stampedToken.token }); + await conn.callAsync('login', { resume: stampedToken.token }); // Trigger onLoginFailure callbacks, this will not include the user object allowLogin = 'bogus'; await test.throwsAsync( async () => - await conn.callAsync('login', { returnServerPromise: true }, { resume: "bogus" }), '403'); + await conn.callAsync('login', { resume: "bogus" }), '403'); // test a forced login fail which WILL include the user object allowLogin = false; await test.throwsAsync( async () => - await conn.callAsync('login', { returnServerPromise: true }, { resume: stampedToken.token }), '403'); + await conn.callAsync('login', { resume: stampedToken.token }), '403'); // Trigger onLogout callbacks const onLogoutExpectedUserId = userId; diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index 6d80c80552..34a335bdc8 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -292,7 +292,7 @@ if (Meteor.isClient) (() => { }, // Make sure the new user has not been inserted async function (test) { - const result = await Meteor.callAsync('countUsersOnServer', { returnServerPromise: true }, { + const result = await Meteor.callAsync('countUsersOnServer', { username: this.newUsername, }); test.equal(result, 0); @@ -400,7 +400,7 @@ if (Meteor.isClient) (() => { }, // Make sure the new user has not been inserted async function (test) { - const result = await Meteor.callAsync('countUsersOnServer', { returnServerPromise: true }, { + const result = await Meteor.callAsync('countUsersOnServer', { 'emails.address': this.newEmail, }); test.equal(result, 0); @@ -428,7 +428,7 @@ if (Meteor.isClient) (() => { })); }, async function (test) { - const token = await Meteor.callAsync("getResetToken", { returnServerPromise: true }); + const token = await Meteor.callAsync("getResetToken"); test.isTrue(token); this.token = token; }, @@ -452,7 +452,7 @@ if (Meteor.isClient) (() => { loggedInAs(this.username, test, expect)); }, async function (test) { - const token = await Meteor.callAsync("getResetToken", { returnServerPromise: true }); + const token = await Meteor.callAsync("getResetToken"); test.isFalse(token); }, logoutStep, @@ -1253,7 +1253,7 @@ if (Meteor.isServer) (() => { test.fail('observe should be gone'); }) - const result = await clientConn.callAsync('login', { returnServerPromise: true }, { + const result = await clientConn.callAsync('login', { user: { username: username }, password: hashPassword('password') }); diff --git a/packages/ddp-client/client/client.js b/packages/ddp-client/client/client.js index fd9c746bbc..4e513da1b2 100644 --- a/packages/ddp-client/client/client.js +++ b/packages/ddp-client/client/client.js @@ -4,3 +4,4 @@ import '../common/livedata_connection'; // Initialize the default server connection and put it on Meteor.connection import './client_convenience'; +import './queueStubsHelpers'; diff --git a/packages/ddp-client/client/queueStubsHelpers.js b/packages/ddp-client/client/queueStubsHelpers.js new file mode 100644 index 0000000000..745735c7d4 --- /dev/null +++ b/packages/ddp-client/client/queueStubsHelpers.js @@ -0,0 +1,216 @@ +import { DDP } from '../common/namespace.js'; +import { isEmpty, last } from "meteor/ddp-common/utils.js"; + +// https://forums.meteor.com/t/proposal-to-fix-issues-with-async-method-stubs/60826 + +let queueSize = 0; +let queue = Promise.resolve(); + +function queueFunction(fn, promiseProps = {}) { + queueSize += 1; + + let resolve; + let reject; + const promise = new Promise((_resolve, _reject) => { + resolve = _resolve; + reject = _reject; + }); + + queue = queue.finally(() => { + fn(resolve, reject); + return promise; + }); + + promise.finally(() => { + queueSize -= 1; + if (queueSize === 0) { + Meteor.connection._maybeMigrate(); + } + }); + + promise.stubPromise = promiseProps.stubPromise; + promise.serverPromise = promiseProps.serverPromise; + + return promise; +} + +let oldReadyToMigrate = Meteor.connection._readyToMigrate; +Meteor.connection._readyToMigrate = function () { + if (queueSize > 0) { + return false; + } + + return oldReadyToMigrate.apply(this, arguments); +}; + +let currentMethodInvocation = null; + +/** + * Meteor sets CurrentMethodInvocation to undefined for the reasons explained at + * https://github.com/meteor/meteor/blob/c9e3551b9673a7ed607f18cb1128563ff49ca96f/packages/ddp-client/common/livedata_connection.js#L578-L605 + * The app code could call `.then` on a promise while the async stub is running, + * causing the `then` callback to think it is inside the stub. + * + * With the queueing we are doing, this is no longer necessary. The point + * of the queueing is to prevent app/package code from running while + * the stub is running, so we don't need to worry about this. + */ + +let oldApplyAsync = Meteor.connection.applyAsync; +Meteor.connection.applyAsync = function () { + let args = arguments; + let name = args[0]; + + if (currentMethodInvocation) { + DDP._CurrentMethodInvocation._set(currentMethodInvocation); + currentMethodInvocation = null; + } + + const enclosing = DDP._CurrentMethodInvocation.get(); + const alreadyInSimulation = enclosing?.isSimulation; + const isFromCallAsync = enclosing?._isFromCallAsync; + + if ( + Meteor.connection._getIsSimulation({ + isFromCallAsync, + alreadyInSimulation, + }) + ) { + // In stub - call immediately + return oldApplyAsync.apply(this, args); + } + + const applyAsyncPromise = oldApplyAsync.apply(this, args); + return queueFunction( + (resolve, reject) => { + let finished = false; + + Meteor._setImmediate(() => { + applyAsyncPromise + .then((result) => { + finished = true; + resolve(result); + }) + .catch((err) => { + finished = true; + reject(err); + }); + }); + + Meteor._setImmediate(() => { + if (!finished) { + console.warn( + `Method stub (${name}) took too long and could cause unexpected problems. Learn more at https://github.com/zodern/fix-async-stubs/#limitations` + ); + } + }); + }, + { + stubPromise: applyAsyncPromise.stubPromise, + serverPromise: applyAsyncPromise.serverPromise, + } + ); +}; + +let oldApply = Meteor.connection.apply; +Meteor.connection.apply = function () { + // [name, args, options] + let options = arguments[2] || {}; + let wait = options.wait; + + // Apply runs the stub before synchronously returning. + // + // However, we want the server to run the methods in the original call order + // so we have to queue sending the message to the server until any previous async + // methods run. + // This does mean the stubs run in a different order than the methods on the + // server. + // TODO: can we queue Meteor.apply in some situations instead of running + // immediately? + + let oldOutstandingMethodBlocks = Meteor.connection._outstandingMethodBlocks; + // Meteor only sends the method if _outstandingMethodBlocks.length is 1. + // Add a wait block to force Meteor to put the new method in a second block. + let outstandingMethodBlocks = [{ wait: true, methods: [] }]; + Meteor.connection._outstandingMethodBlocks = outstandingMethodBlocks; + + let result; + try { + result = oldApply.apply(this, arguments); + } finally { + Meteor.connection._outstandingMethodBlocks = oldOutstandingMethodBlocks; + } + + if (outstandingMethodBlocks[1]) { + let methodInvoker = outstandingMethodBlocks[1].methods[0]; + + if (methodInvoker) { + queueMethodInvoker(methodInvoker, wait); + } + } + + return result; +}; + +function queueMethodInvoker(methodInvoker, wait) { + queueFunction((resolve) => { + let self = Meteor.connection; + // based on https://github.com/meteor/meteor/blob/e0631738f2a8a914d8a50b1060e8f40cb0873680/packages/ddp-client/common/livedata_connection.js#L833-L853C1 + if (wait) { + // It's a wait method! Wait methods go in their own block. + self._outstandingMethodBlocks.push({ + wait: true, + methods: [methodInvoker], + }); + } else { + // Not a wait method. Start a new block if the previous block was a wait + // block, and add it to the last block of methods. + if ( + isEmpty(self._outstandingMethodBlocks) || + last(self._outstandingMethodBlocks).wait + ) { + self._outstandingMethodBlocks.push({ + wait: false, + methods: [], + }); + } + + last(self._outstandingMethodBlocks).methods.push(methodInvoker); + } + + // If we added it to the first block, send it out now. + if (self._outstandingMethodBlocks.length === 1) methodInvoker.sendMessage(); + + resolve(); + }); +} + +/** + * Queue subscriptions in case they rely on previous method calls + */ +let queueSend = false; +let oldSubscribe = Meteor.connection.subscribe; +Meteor.connection.subscribe = function () { + queueSend = true; + try { + return oldSubscribe.apply(this, arguments); + } finally { + queueSend = false; + } +}; + +let oldSend = Meteor.connection._send; +Meteor.connection._send = function () { + if (!queueSend) { + return oldSend.apply(this, arguments); + } + + queueSend = false; + queueFunction((resolve) => { + try { + oldSend.apply(this, arguments); + } finally { + resolve(); + } + }); +}; diff --git a/packages/ddp-client/common/livedata_connection.js b/packages/ddp-client/common/livedata_connection.js index a49cea44f7..ba242e0da3 100644 --- a/packages/ddp-client/common/livedata_connection.js +++ b/packages/ddp-client/common/livedata_connection.js @@ -579,71 +579,7 @@ export class Connection { ); } - const applyOptions = ['returnStubValue', 'returnServerResultPromise', 'returnServerPromise']; - const defaultOptions = { - returnServerResultPromise: true, - }; - const options = { - ...defaultOptions, - ...(applyOptions.some(o => args[0]?.hasOwnProperty(o)) - ? args.shift() - : {}), - }; - - const invocation = DDP._CurrentCallAsyncInvocation.get(); - - if (invocation?.hasCallAsyncParent) { - return this.applyAsync(name, args, { ...options, isFromCallAsync: true }); - } - - /* - * This is necessary because when you call a Promise.then, you're actually calling a bound function by Meteor. - * - * This is done by this code https://github.com/meteor/meteor/blob/17673c66878d3f7b1d564a4215eb0633fa679017/npm-packages/meteor-promise/promise_client.js#L1-L16. (All the logic below can be removed in the future, when we stop overwriting the - * Promise.) - * - * When you call a ".then()", like "Meteor.callAsync().then()", the global context (inside currentValues) - * will be from the call of Meteor.callAsync(), and not the context after the promise is done. - * - * This means that without this code if you call a stub inside the ".then()", this stub will act as a simulation - * and won't reach the server. - * - * Inside the function _getIsSimulation(), if isFromCallAsync is false, we continue to consider just the - * alreadyInSimulation, otherwise, isFromCallAsync is true, we also check the value of callAsyncMethodRunning (by - * calling DDP._CurrentMethodInvocation._isCallAsyncMethodRunning()). - * - * With this, if a stub is running inside a ".then()", it'll know it's not a simulation, because callAsyncMethodRunning - * will be false. - * - * DDP._CurrentMethodInvocation._set() is important because without it, if you have a code like: - * - * Meteor.callAsync("m1").then(() => { - * Meteor.callAsync("m2") - * }) - * - * The call the method m2 will act as a simulation and won't reach the server. That's why we reset the context here - * before calling everything else. - * - * */ - DDP._CurrentMethodInvocation._set(); - DDP._CurrentMethodInvocation._setCallAsyncMethodRunning(true); - let applyAsyncPromise = {}; - const promise = new Promise((resolve, reject) => { - DDP._CurrentCallAsyncInvocation._set({ name, hasCallAsyncParent: true }); - const p = this.applyAsync(name, args, { isFromCallAsync: true, ...options }) - p.then(resolve) - .catch(reject) - .finally(() => { - DDP._CurrentCallAsyncInvocation._set(); - DDP._CurrentMethodInvocation._setCallAsyncMethodRunning(false); - }); - applyAsyncPromise = p; - }); - - promise.stubPromise = applyAsyncPromise.stubPromise; - promise.serverPromise = applyAsyncPromise.serverPromise; - - return promise; + return this.applyAsync(name, args, { returnServerResultPromise: true }); } /** diff --git a/packages/ddp-client/test/livedata_tests.js b/packages/ddp-client/test/livedata_tests.js index 248e748c2f..0423a99534 100644 --- a/packages/ddp-client/test/livedata_tests.js +++ b/packages/ddp-client/test/livedata_tests.js @@ -1213,11 +1213,13 @@ testAsyncMulti('livedata - methods with nested stubs', [ }, ]); - Tinytest.addAsync('livedata - isAsync call', async function (test) { - Meteor.call('isCallAsync', (err, result) => test.equal(result, false)) - const result = await Meteor.callAsync('isCallAsync', { returnStubValue: true }) - test.equal(result, true) -}) +// TODO [FIBERS] - check if this still makes sense to have + +// Tinytest.addAsync('livedata - isAsync call', async function (test) { +// Meteor.call('isCallAsync', (err, result) => test.equal(result, false)) +// const result = await Meteor.callAsync('isCallAsync', { returnStubValue: true }) +// test.equal(result, true) +// }) // XXX some things to test in greater detail: // staying in simulation mode