Feedback from review

src/Mjolnir.ts

@@ -26,7 +26,7 @@ import {
     UserID
 } from "matrix-bot-sdk";
 
-import BanList, { ALL_RULE_TYPES, ListRuleChange, RULE_ROOM, RULE_SERVER, RULE_USER } from "./models/BanList";
+import BanList, { ALL_RULE_TYPES as ALL_BAN_LIST_RULE_TYPES, ListRuleChange, RULE_ROOM, RULE_SERVER, RULE_USER } from "./models/BanList";
 import { applyServerAcls } from "./actions/ApplyAcl";
 import { RoomUpdateError } from "./models/RoomUpdateError";
 import { COMMAND_PREFIX, handleCommand } from "./commands/CommandHandler";
@@ -764,7 +764,7 @@ export class Mjolnir {
     /**
      * Pulls any changes to the rules that are in a policy room and updates all protected rooms
      * with those changes. Does not fail if there are errors updating the room, these are reported to the management room.
-     * @param policyRoomId The room with a policy list which we will check for changes and apply them to all protected rooms.
+     * @param banList The `BanList` which we will check for changes and apply them to all protected rooms.
      * @returns When all of the protected rooms have been updated.
      */
      public async syncWithBanList(banList: BanList): Promise<void> {
@@ -809,7 +809,7 @@ export class Mjolnir {
         // themselves.
         const banList = this.banLists.find(list => list.roomId === roomId);
         if (banList !== undefined) {
-            if (ALL_RULE_TYPES.includes(event['type'])) {
+            if (ALL_BAN_LIST_RULE_TYPES.includes(event['type']) || event['type'] === 'm.room.redaction') {
                 banList.updateForEvent(event)
             }
         }

src/models/BanList.ts

@@ -72,8 +72,10 @@ export interface ListRuleChange {
 }
 
 declare interface BanList {
+    // BanList.update is emitted when the BanList has pulled new rules from Matrix and informs listeners of any changes.
     on(event: 'BanList.update', listener: (list: BanList, changes: ListRuleChange[]) => void): this
     emit(event: 'BanList.update', list: BanList, changes: ListRuleChange[]): boolean
+    // BanList.batch is emitted when the BanList has created a batch from the events provided by `updateForEvent`.
     on(event: 'BanList.batch', listener: (list: BanList) => void): this
     emit(event: 'BanList.batch', list: BanList): boolean
 }
@@ -292,11 +294,6 @@ class BanList extends EventEmitter {
      * @param event An event from the room the `BanList` models to inform an instance about.
      */
     public updateForEvent(event: { event_id: string }): void {
-        // We have to allow the batcher to emit BanList.batch because
-        // if we await in the updateForEvent method that is called by Mjolnir's sync
-        // event emitter, then by the time we start batching we will be far too late
-        // and unable to batch effectivly
-        // if you don't believe me you can test it for yourself, it is rubbish.
         this.batcher.addToBatch(event.event_id)
     }
 }
@@ -308,10 +305,12 @@ export default BanList;
  * out of the events given to `addToBatch`.
  */
 class UpdateBatcher {
+    // Whether we are waiting for more events to form a batch.
     private isWaiting = false;
-    private previousEventId: string|null = null;
+    // The latest (or most recent) event we have received.
-    private readonly waitPeriod = 200; // 200ms seems good enough.
+    private latestEventId: string|null = null;
-    private readonly maxWait = 3000; // 3s is long enough to wait while batching.
+    private readonly waitPeriodMS = 200; // 200ms seems good enough.
+    private readonly maxWaitMS = 3000; // 3s is long enough to wait while batching.
 
     constructor(private readonly banList: BanList) {
 
@@ -321,7 +320,7 @@ class UpdateBatcher {
      * Reset the state for the next batch.
      */
     private reset() {
-        this.previousEventId = null;
+        this.latestEventId = null;
         this.isWaiting = false;
     }
 
@@ -334,8 +333,8 @@ class UpdateBatcher {
     private async checkBatch(eventId: string): Promise<void> {
         let start = Date.now();
         do {
-            await new Promise(resolve => setTimeout(resolve, this.waitPeriod));
+            await new Promise(resolve => setTimeout(resolve, this.waitPeriodMS));
-        } while ((Date.now() - start) < this.maxWait && this.previousEventId !== eventId)
+        } while ((Date.now() - start) < this.maxWaitMS && this.latestEventId !== eventId)
         this.reset();
         this.banList.emit('BanList.batch', this.banList);
     }
@@ -346,11 +345,18 @@ class UpdateBatcher {
      */
     public addToBatch(eventId: string): void {
         if (this.isWaiting) {
-            this.previousEventId = eventId;
+            this.latestEventId = eventId;
             return;
         }
-        this.previousEventId = eventId;
+        this.latestEventId = eventId;
         this.isWaiting = true;
+        // We 'spawn' off here after performing the checks above
+        // rather than before (ie if `addToBatch` was async) because
+        // `banListTest` showed that there were 100~ ACL events per protected room
+        // as compared to just 5~ by doing this. Not entirely sure why but it probably
+        // has to do with queuing up `n event` tasks on the event loop that exaust scheduling
+        // (so the latency between them is percieved as much higher by
+        // the time they get checked in `this.checkBatch`, thus batching fails).
         this.checkBatch(eventId);
     }
 }

test/integration/banListTest.ts

@@ -237,7 +237,8 @@ describe.only('Test: ACL updates will batch when rules are added in succession.'
         const moderator = await newTestUser({ name: { contains: "moderator" }});
         moderator.joinRoom(this.mjolnir.managementRoomId);
         const mjolnirId = await mjolnir.getUserId();
-        // How many rooms are we going to protect? Do we need to protect a few and verify all the ACL updates were batched in each?
+
+        // Setup some protected rooms so we can check their ACL state later.
         const protectedRooms: string[] = [];
         for (let i = 0; i < 10; i++) {
             const room = await moderator.createRoom({ invite: [mjolnirId]});
@@ -255,22 +256,24 @@ describe.only('Test: ACL updates will batch when rules are added in succession.'
             assert.equal(roomAcl?.deny?.length ?? 0, 0, 'There should be no entries in the deny ACL.');
         }));
 
-        // Flood the subsribed list with banned servers, which should prompt Mjolnir to update server ACL in protected rooms.
+        // Flood the watched list with banned servers, which should prompt Mjolnir to update server ACL in protected rooms.
         const banListId = await moderator.createRoom({ invite: [mjolnirId] });
         mjolnir.joinRoom(banListId);
         this.mjolnir!.watchList(Permalinks.forRoom(banListId));
         const acl = new ServerAcl(serverName).denyIpAddresses().allowServer("*");
         for (let i = 0; i < 200; i++) {
-            acl.denyServer(i.toString());
+            const badServer = `${i}.evil.com`;
-            await createPolicyRule(moderator, banListId, RULE_SERVER, `${i}`, `Rule #${i}`);
+            acl.denyServer(badServer);
+            await createPolicyRule(moderator, banListId, RULE_SERVER, badServer, `Rule #${i}`);
             // Give them a bit of a spread over time.
             await new Promise(resolve => setTimeout(resolve, 5));
         }
 
         // We do this because it should force us to wait until all the ACL events have been applied.
+        // Even if that does mean the last few events will not go through batching...
         await this.mjolnir!.syncLists();
 
-        // Check each of the protected rooms.
+        // Check each of the protected rooms for ACL events and make sure they were batched and are correct.
         await Promise.all(protectedRooms.map(async room => {
             const roomAcl = await mjolnir.getRoomStateEvent(room, "m.room.server_acl", "");
             if (!acl.matches(roomAcl)) {
@@ -281,8 +284,8 @@ describe.only('Test: ACL updates will batch when rules are added in succession.'
                 events.forEach(event => event.type === 'm.room.server_acl' ? aclEventCount += 1 : null);
             });
             LogService.debug('BanListTest', `aclEventCount: ${aclEventCount}`);
-            // If there's less than two then it probably means the ACL was put in by this test calling syncLists
+            // If there's less than two then it means the ACL was updated by this test calling `this.mjolnir!.syncLists()`
-            // and not the listener that detects changes to ban lists.
+            // and not the listener that detects changes to ban lists (that we want to test!).
             assert.equal(aclEventCount < 10 && aclEventCount > 2, true, 'We should have sent less than 10 ACL events to each room because they should be batched')
         }));
     })