diff --git a/package.json b/package.json index f3c06fb6bc6..ed4880c40c4 100644 --- a/package.json +++ b/package.json @@ -55,6 +55,7 @@ "bs58": "^4.0.1", "content-type": "^1.0.4", "loglevel": "^1.7.1", + "p-retry": "^4.5.0", "qs": "^6.9.6", "request": "^2.88.2", "unhomoglyph": "^1.0.6" diff --git a/spec/unit/models/MSC3089TreeSpace.spec.ts b/spec/unit/models/MSC3089TreeSpace.spec.ts index a99140036bb..a7f42e07bcb 100644 --- a/spec/unit/models/MSC3089TreeSpace.spec.ts +++ b/spec/unit/models/MSC3089TreeSpace.spec.ts @@ -25,6 +25,7 @@ import { } from "../../../src/models/MSC3089TreeSpace"; import { DEFAULT_ALPHABET } from "../../../src/utils"; import { MockBlob } from "../../MockBlob"; +import { MatrixError } from "../../../src/http-api"; describe("MSC3089TreeSpace", () => { let client: MatrixClient; @@ -93,7 +94,7 @@ describe("MSC3089TreeSpace", () => { }); client.sendStateEvent = fn; await tree.setName(newName); - expect(fn.mock.calls.length).toBe(1); + expect(fn).toHaveBeenCalledTimes(1); }); it('should support inviting users to the space', async () => { @@ -104,8 +105,62 @@ describe("MSC3089TreeSpace", () => { return Promise.resolve(); }); client.invite = fn; - await tree.invite(target); - expect(fn.mock.calls.length).toBe(1); + await tree.invite(target, false); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('should retry invites to the space', async () => { + const target = targetUser; + const fn = jest.fn().mockImplementation((inviteRoomId: string, userId: string) => { + expect(inviteRoomId).toEqual(roomId); + expect(userId).toEqual(target); + if (fn.mock.calls.length === 1) return Promise.reject(new Error("Sample Failure")); + return Promise.resolve(); + }); + client.invite = fn; + await tree.invite(target, false); + expect(fn).toHaveBeenCalledTimes(2); + }); + + it('should not retry invite permission errors', async () => { + const target = targetUser; + const fn = jest.fn().mockImplementation((inviteRoomId: string, userId: string) => { + expect(inviteRoomId).toEqual(roomId); + expect(userId).toEqual(target); + return Promise.reject(new MatrixError({ errcode: "M_FORBIDDEN", error: "Sample Failure" })); + }); + client.invite = fn; + try { + await tree.invite(target, false); + + // noinspection ExceptionCaughtLocallyJS + throw new Error("Failed to fail"); + } catch (e) { + expect(e.errcode).toEqual("M_FORBIDDEN"); + } + + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('should invite to subspaces', async () => { + const target = targetUser; + const fn = jest.fn().mockImplementation((inviteRoomId: string, userId: string) => { + expect(inviteRoomId).toEqual(roomId); + expect(userId).toEqual(target); + return Promise.resolve(); + }); + client.invite = fn; + tree.getDirectories = () => [ + // Bare minimum overrides. We proxy to our mock function manually so we can + // count the calls, not to ensure accuracy. The invite function behaving correctly + // is covered by another test. + { invite: (userId) => fn(tree.roomId, userId) } as MSC3089TreeSpace, + { invite: (userId) => fn(tree.roomId, userId) } as MSC3089TreeSpace, + { invite: (userId) => fn(tree.roomId, userId) } as MSC3089TreeSpace, + ]; + + await tree.invite(target, true); + expect(fn).toHaveBeenCalledTimes(4); }); async function evaluatePowerLevels(pls: any, role: TreePermissions, expectedPl: number) { diff --git a/spec/unit/utils.spec.ts b/spec/unit/utils.spec.ts index 76123d1ca86..a062f7df4ab 100644 --- a/spec/unit/utils.spec.ts +++ b/spec/unit/utils.spec.ts @@ -7,6 +7,7 @@ import { lexicographicCompare, nextString, prevString, + simpleRetryOperation, stringToBase, } from "../../src/utils"; import { logger } from "../../src/logger"; @@ -267,6 +268,34 @@ describe("utils", function() { }); }); + describe('simpleRetryOperation', () => { + it('should retry', async () => { + let count = 0; + const val = {}; + const fn = (attempt) => { + count++; + + // If this expectation fails then it can appear as a Jest Timeout due to + // the retry running beyond the test limit. + expect(attempt).toEqual(count); + + if (count > 1) { + return Promise.resolve(val); + } else { + return Promise.reject(new Error("Iterative failure")); + } + }; + + const ret = await simpleRetryOperation(fn); + expect(ret).toBe(val); + expect(count).toEqual(2); + }); + + // We don't test much else of the function because then we're just testing that the + // underlying library behaves, which should be tested on its own. Our API surface is + // all that concerns us. + }); + describe('DEFAULT_ALPHABET', () => { it('should be usefully printable ASCII in order', () => { expect(DEFAULT_ALPHABET).toEqual( diff --git a/src/models/MSC3089TreeSpace.ts b/src/models/MSC3089TreeSpace.ts index f36642a8f79..5b4771b27c0 100644 --- a/src/models/MSC3089TreeSpace.ts +++ b/src/models/MSC3089TreeSpace.ts @@ -19,8 +19,16 @@ import { EventType, IEncryptedFile, MsgType, UNSTABLE_MSC3089_BRANCH, UNSTABLE_M import { Room } from "./room"; import { logger } from "../logger"; import { MatrixEvent } from "./event"; -import { averageBetweenStrings, DEFAULT_ALPHABET, lexicographicCompare, nextString, prevString } from "../utils"; +import { + averageBetweenStrings, + DEFAULT_ALPHABET, + lexicographicCompare, + nextString, + prevString, + simpleRetryOperation, +} from "../utils"; import { MSC3089Branch } from "./MSC3089Branch"; +import promiseRetry from "p-retry"; /** * The recommended defaults for a tree space's power levels. Note that this @@ -110,12 +118,29 @@ export class MSC3089TreeSpace { * Invites a user to the tree space. They will be given the default Viewer * permission level unless specified elsewhere. * @param {string} userId The user ID to invite. + * @param {boolean} andSubspaces True (default) to invite the user to all + * directories/subspaces too, recursively. * @returns {Promise} Resolves when complete. */ - public invite(userId: string): Promise { - // TODO: [@@TR] Reliable invites + public invite(userId: string, andSubspaces = true): Promise { // TODO: [@@TR] Share keys - return this.client.invite(this.roomId, userId); + const promises: Promise[] = [this.retryInvite(userId)]; + if (andSubspaces) { + promises.push(...this.getDirectories().map(d => d.invite(userId, andSubspaces))); + } + return Promise.all(promises).then(); // .then() to coerce types + } + + private retryInvite(userId: string): Promise { + return simpleRetryOperation(() => { + return this.client.invite(this.roomId, userId).catch(e => { + // We don't want to retry permission errors forever... + if (e?.errcode === "M_FORBIDDEN") { + throw new promiseRetry.AbortError(e); + } + throw e; + }); + }); } /** diff --git a/src/utils.ts b/src/utils.ts index 50c5b9f39fa..a2504a011c3 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -20,7 +20,8 @@ limitations under the License. * @module utils */ -import unhomoglyph from 'unhomoglyph'; +import unhomoglyph from "unhomoglyph"; +import promiseRetry from "p-retry"; /** * Encode a dictionary of query parameters. @@ -443,6 +444,26 @@ export async function chunkPromises(fns: (() => Promise)[], chunkSize: num return results; } +/** + * Retries the function until it succeeds or is interrupted. The given function must return + * a promise which throws/rejects on error, otherwise the retry will assume the request + * succeeded. The promise chain returned will contain the successful promise. The given function + * should always return a new promise. + * @param {Function} promiseFn The function to call to get a fresh promise instance. Takes an + * attempt count as an argument, for logging/debugging purposes. + * @returns {Promise} The promise for the retried operation. + */ +export function simpleRetryOperation(promiseFn: (attempt: number) => Promise): Promise { + return promiseRetry((attempt: number) => { + return promiseFn(attempt); + }, { + forever: true, + factor: 2, + minTimeout: 3000, // ms + maxTimeout: 15000, // ms + }); +} + // We need to be able to access the Node.js crypto library from within the // Matrix SDK without needing to `require("crypto")`, which will fail in // browsers. So `index.ts` will call `setCrypto` to store it, and when we need diff --git a/yarn.lock b/yarn.lock index 04057af8baf..b582d75fbc2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1132,6 +1132,7 @@ "@matrix-org/olm@https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.3.tgz": version "3.2.3" + uid cc332fdd25c08ef0e40f4d33fc3f822a0f98b6f4 resolved "https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.3.tgz#cc332fdd25c08ef0e40f4d33fc3f822a0f98b6f4" "@nicolo-ribaudo/chokidar-2@2.1.8-no-fsevents": @@ -1305,6 +1306,11 @@ "@types/tough-cookie" "*" form-data "^2.5.0" +"@types/retry@^0.12.0": + version "0.12.0" + resolved "https://registry.yarnpkg.com/@types/retry/-/retry-0.12.0.tgz#2b35eccfcee7d38cd72ad99232fbd58bffb3c84d" + integrity sha512-wWKOClTTiizcZhXnPY4wikVAwmdYHp8q6DmC+EJUzAMsycb7HB32Kh9RN4+0gExjmPmZSAQjgURXIGATPegAvA== + "@types/stack-utils@^2.0.0": version "2.0.0" resolved "https://registry.yarnpkg.com/@types/stack-utils/-/stack-utils-2.0.0.tgz#7036640b4e21cc2f259ae826ce843d277dad8cff" @@ -5220,6 +5226,14 @@ p-locate@^4.1.0: dependencies: p-limit "^2.2.0" +p-retry@^4.5.0: + version "4.5.0" + resolved "https://registry.yarnpkg.com/p-retry/-/p-retry-4.5.0.tgz#6685336b3672f9ee8174d3769a660cb5e488521d" + integrity sha512-5Hwh4aVQSu6BEP+w2zKlVXtFAaYQe1qWuVADSgoeVlLjwe/Q/AMSoRR4MDeaAfu8llT+YNbEijWu/YF3m6avkg== + dependencies: + "@types/retry" "^0.12.0" + retry "^0.12.0" + p-try@^2.0.0: version "2.2.0" resolved "https://registry.yarnpkg.com/p-try/-/p-try-2.2.0.tgz#cb2868540e313d61de58fafbe35ce9004d5540e6" @@ -5943,6 +5957,11 @@ ret@~0.1.10: resolved "https://registry.yarnpkg.com/ret/-/ret-0.1.15.tgz#b8a4825d5bdb1fc3f6f53c2bc33f81388681c7bc" integrity sha512-TTlYpa+OL+vMMNG24xSlQGEJ3B/RzEfUlLct7b5G/ytav+wPrplCpVMFuwzXbkecJrb6IYo1iFb0S9v37754mg== +retry@^0.12.0: + version "0.12.0" + resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b" + integrity sha1-G0KmJmoh8HQh0bC1S33BZ7AcATs= + reusify@^1.0.4: version "1.0.4" resolved "https://registry.yarnpkg.com/reusify/-/reusify-1.0.4.tgz#90da382b1e126efc02146e90845a88db12925d76"