Making IgnoredInvites mostly sync.

Yoric · Yoric · commit e1e0996efd48 · 2022-09-08T10:38:11.000+02:00
Experience and feedback from matrix-org/matrix-react-sdk#9255 indicates that we cannot integrate an async `IgnoredInvites.getRuleForInvite`. This PR: - rewrites `getRuleForInvite` to be sync; - adds some locking within `IgnoredInvites` to avoid race conditions noticed by @t3chguy.
diff --git a/package.json b/package.json
@@ -55,6 +55,7 @@
   "dependencies": {
     "@babel/runtime": "^7.12.5",
     "another-json": "^0.2.0",
+    "await-lock": "^2.2.2",
     "browser-request": "^0.3.3",
     "bs58": "^5.0.0",
     "content-type": "^1.0.4",
diff --git a/spec/unit/matrix-client.spec.ts b/spec/unit/matrix-client.spec.ts
@@ -1509,12 +1509,25 @@ describe("MatrixClient", function() {
             expect(target1).toBe(target2);
         });
 
-        it("should initialize and return the same `sources` consistently", async () => {
-            const sources1 = await client.ignoredInvites.getOrCreateSourceRooms();
-            const sources2 = await client.ignoredInvites.getOrCreateSourceRooms();
-            expect(sources1).toBeTruthy();
+        it("if there are no source rooms, it should return an empty list consistently", async () => {
+            const sources1 = client.ignoredInvites.getSourceRooms();
+            const sources2 = client.ignoredInvites.getSourceRooms();
+            expect(sources1).toHaveLength(0);
+            expect(sources1).toEqual(sources2);
+        });
+
+        it("if there are any source rooms, it should the same list consistently", async () => {
+            const NEW_SOURCE_ROOM_ID = "!another-source:example.org";
+
+            // Make sure that everything is initialized.
+            await client.joinRoom(NEW_SOURCE_ROOM_ID);
+            await client.ignoredInvites.addSource(NEW_SOURCE_ROOM_ID);
+
+            const sources1 = client.ignoredInvites.getSourceRooms();
+            const sources2 = client.ignoredInvites.getSourceRooms();
             expect(sources1).toHaveLength(1);
             expect(sources1).toEqual(sources2);
+            expect(sources1.map(room => room.roomId)).toContain(NEW_SOURCE_ROOM_ID);
         });
 
         it("should initially not reject any invite", async () => {
@@ -1526,6 +1539,7 @@ describe("MatrixClient", function() {
         });
 
         it("should reject invites once we have added a matching rule in the target room (scope: user)", async () => {
+            await client.ignoredInvites.getOrCreateTargetRoom();
             await client.ignoredInvites.addRule(PolicyScope.User, "*:example.org", "just a test");
 
             // We should reject this invite.
@@ -1615,7 +1629,6 @@ describe("MatrixClient", function() {
             const NEW_SOURCE_ROOM_ID = "!another-source:example.org";
 
             // Make sure that everything is initialized.
-            await client.ignoredInvites.getOrCreateSourceRooms();
             await client.joinRoom(NEW_SOURCE_ROOM_ID);
             await client.ignoredInvites.addSource(NEW_SOURCE_ROOM_ID);
 
@@ -1678,8 +1691,8 @@ describe("MatrixClient", function() {
             const NEW_SOURCE_ROOM_ID = "!another-source:example.org";
 
             // Make sure that everything is initialized.
-            await client.ignoredInvites.getOrCreateSourceRooms();
             await client.joinRoom(NEW_SOURCE_ROOM_ID);
+            await client.ignoredInvites.getOrCreateTargetRoom();
             const newSourceRoom = client.getRoom(NEW_SOURCE_ROOM_ID);
 
             // Fetch the list of sources and check that we do not have the new room yet.
diff --git a/src/models/invites-ignorer.ts b/src/models/invites-ignorer.ts
@@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+import AwaitLock from "await-lock";
 import { UnstableValue } from "matrix-events-sdk";
 
 import { MatrixClient } from "../client";
@@ -71,6 +72,16 @@ export enum PolicyScope {
  * our data structures.
  */
 export class IgnoredInvites {
+    // A lock around method `getOrCreateTargetRoom`.
+    // Used to ensure that only one async task of this class
+    // is creating a new target room and modifying the
+    // `target` property of account key `IGNORE_INVITES_POLICIES`.
+    private _getOrCreateTargetRoomLock = new AwaitLock();
+
+    // A lock around method `withIgnoreInvitesPoliciesLock`.
+    // Used to ensure that only one async task of this class is
+    // modifying `IGNORE_INVITES_POLICIES` at any point in time.
+    private _withIgnoreInvitesPoliciesLock = new AwaitLock();
     constructor(
         private readonly client: MatrixClient,
     ) {
@@ -83,6 +94,12 @@ export class IgnoredInvites {
      * @param entity The entity covered by this rule. Globs are supported.
      * @param reason A human-readable reason for introducing this new rule.
      * @return The event id for the new rule.
+     *
+     * # Safety
+     *
+     * This method will rewrite the `Policies` object in the user's account data.
+     * This rewrite is inherently racy and could overwrite or be overwritten by
+     * other concurrent rewrites of the same object.
      */
     public async addRule(scope: PolicyScope, entity: string, reason: string): Promise<string> {
         const target = await this.getOrCreateTargetRoom();
@@ -119,11 +136,10 @@ export class IgnoredInvites {
      */
     public async addSource(roomId: string): Promise<boolean> {
         // We attempt to join the room *before* calling
-        // `await this.getOrCreateSourceRooms()` to decrease the duration
+        // `await this.getSourceRooms()` to decrease the duration
         // of the racy section.
         await this.client.joinRoom(roomId);
-        // Race starts.
-        const sources = (await this.getOrCreateSourceRooms())
+        const sources = this.getSourceRooms()
             .map(room => room.roomId);
         if (sources.includes(roomId)) {
             return false;
@@ -133,7 +149,6 @@ export class IgnoredInvites {
             ignoreInvitesPolicies.sources = sources;
         });
 
-        // Race ends.
         return true;
     }
 
@@ -144,10 +159,10 @@ export class IgnoredInvites {
      * @param roomId The room to which the user is invited.
      * @returns A rule matching the entity, if any was found, `null` otherwise.
      */
-    public async getRuleForInvite({ sender, roomId }: {
+    public getRuleForInvite({ sender, roomId }: {
         sender: string;
         roomId: string;
-    }): Promise<Readonly<MatrixEvent | null>> {
+    }): Readonly<MatrixEvent | null> {
         // In this implementation, we perform a very naive lookup:
         // - search in each policy room;
         // - turn each (potentially glob) rule entity into a regexp.
@@ -158,7 +173,7 @@ export class IgnoredInvites {
         // - match several entities per go;
         // - pre-compile each rule entity into a regexp;
         // - pre-compile entire rooms into a single regexp.
-        const policyRooms = await this.getOrCreateSourceRooms();
+        const policyRooms = this.getSourceRooms();
         const senderServer = sender.split(":")[1];
         const roomServer = roomId.split(":")[1];
         for (const room of policyRooms) {
@@ -227,21 +242,30 @@ export class IgnoredInvites {
             const room = this.client.getRoom(target);
             if (room) {
                 return room;
-            } else {
-                target = null;
             }
         }
-        // We need to create our own policy room for ignoring invites.
-        target = (await this.client.createRoom({
-            name: "Individual Policy Room",
-            preset: Preset.PrivateChat,
-        })).room_id;
-        await this.withIgnoreInvitesPolicies(ignoreInvitesPolicies => {
-            ignoreInvitesPolicies.target = target;
-        });
+        try {
+            // We need to create our own policy room for ignoring invites.
+            await this._getOrCreateTargetRoomLock.acquireAsync();
+            target = (await this.client.createRoom({
+                name: "Individual Policy Room",
+                preset: Preset.PrivateChat,
+            })).room_id;
+            await this.withIgnoreInvitesPolicies(ignoreInvitesPolicies => {
+                ignoreInvitesPolicies.target = target;
+                if (!("sources" in ignoreInvitesPolicies)) {
+                    // `[target]` is a reasonable default for `sources`.
+                    ignoreInvitesPolicies.sources = [target];
+                }
+            });
 
-        // Since we have just called `createRoom`, `getRoom` should not be `null`.
-        return this.client.getRoom(target)!;
+            // Since we have just called `createRoom`, `getRoom` should not be `null`.
+            // Note that this is unavoidably racy, e.g. another client could have left
+            // the room during the call to `this.withIgnoreInvitesPolicies`.
+            return this.client.getRoom(target)!;
+        } finally {
+            this._getOrCreateTargetRoomLock.release();
+        }
     }
 
     /**
@@ -258,40 +282,21 @@ export class IgnoredInvites {
      * This rewrite is inherently racy and could overwrite or be overwritten by
      * other concurrent rewrites of the same object.
      */
-    public async getOrCreateSourceRooms(): Promise<Room[]> {
+    public getSourceRooms(): Room[] {
         const ignoreInvitesPolicies = this.getIgnoreInvitesPolicies();
         let sources = ignoreInvitesPolicies.sources;
 
         // Validate `sources`. If it is invalid, trash out the current `sources`
         // and create a new list of sources from `target`.
-        let hasChanges = false;
         if (!Array.isArray(sources)) {
             // `sources` could not be an array.
-            hasChanges = true;
             sources = [];
         }
-        let sourceRooms: Room[] = sources
+        const sourceRooms: Room[] = sources
             // `sources` could contain non-string / invalid room ids
             .filter(roomId => typeof roomId === "string")
             .map(roomId => this.client.getRoom(roomId))
             .filter(room => !!room);
-        if (sourceRooms.length != sources.length) {
-            hasChanges = true;
-        }
-        if (sourceRooms.length == 0) {
-            // `sources` could be empty (possibly because we've removed
-            // invalid content)
-            const target = await this.getOrCreateTargetRoom();
-            hasChanges = true;
-            sourceRooms = [target];
-        }
-        if (hasChanges) {
-            // Reload `policies`/`ignoreInvitesPolicies` in case it has been changed
-            // during or by our call to `this.getTargetRoom()`.
-            await this.withIgnoreInvitesPolicies(ignoreInvitesPolicies => {
-                ignoreInvitesPolicies.sources = sources;
-            });
-        }
         return sourceRooms;
     }
 
@@ -313,10 +318,15 @@ export class IgnoredInvites {
      * Modify in place the `IGNORE_INVITES_POLICIES` object from account data.
      */
     private async withIgnoreInvitesPolicies(cb: (ignoreInvitesPolicies: {[key: string]: any}) => void) {
-        const { policies, ignoreInvitesPolicies } = this.getPoliciesAndIgnoreInvitesPolicies();
-        cb(ignoreInvitesPolicies);
-        policies[IGNORE_INVITES_ACCOUNT_EVENT_KEY.name] = ignoreInvitesPolicies;
-        await this.client.setAccountData(POLICIES_ACCOUNT_EVENT_TYPE.name, policies);
+        await this._withIgnoreInvitesPoliciesLock.acquireAsync();
+        try {
+            const { policies, ignoreInvitesPolicies } = this.getPoliciesAndIgnoreInvitesPolicies();
+            cb(ignoreInvitesPolicies);
+            policies[IGNORE_INVITES_ACCOUNT_EVENT_KEY.name] = ignoreInvitesPolicies;
+            await this.client.setAccountData(POLICIES_ACCOUNT_EVENT_TYPE.name, policies);
+        } finally {
+            this._withIgnoreInvitesPoliciesLock.release();
+        }
     }
 
     /**
diff --git a/yarn.lock b/yarn.lock
@@ -1410,7 +1410,6 @@
 
 "@matrix-org/olm@https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.12.tgz":
   version "3.2.12"
-  uid "0bce3c86f9d36a4984d3c3e07df1c3fb4c679bd9"
   resolved "https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.12.tgz#0bce3c86f9d36a4984d3c3e07df1c3fb4c679bd9"
 
 "@nicolo-ribaudo/chokidar-2@2.1.8-no-fsevents.3":
@@ -2052,6 +2051,11 @@ available-typed-arrays@^1.0.5:
   resolved "https://registry.yarnpkg.com/available-typed-arrays/-/available-typed-arrays-1.0.5.tgz#92f95616501069d07d10edb2fc37d3e1c65123b7"
   integrity sha512-DMD0KiN46eipeziST1LPP/STfDU0sufISXmjSgvVsoU2tqxctQeASejWcfNtxYKqETM1UxQ8sp2OrSBWpHY6sw==
 
+await-lock@^2.2.2:
+  version "2.2.2"
+  resolved "https://registry.yarnpkg.com/await-lock/-/await-lock-2.2.2.tgz#a95a9b269bfd2f69d22b17a321686f551152bcef"
+  integrity sha512-aDczADvlvTGajTDjcjpJMqRkOF6Qdz3YbPZm/PyW6tKPkx2hlYBzxMhEywM/tU72HrVZjgl5VCdRuMlA7pZ8Gw==
+
 aws-sign2@~0.7.0:
   version "0.7.0"
   resolved "https://registry.yarnpkg.com/aws-sign2/-/aws-sign2-0.7.0.tgz#b46e890934a9591f2d2f6f86d7e6a9f1b3fe76a8"
