Rewrite the quit debouncer to drop quits early

changelog.d/1091.feature

@@ -0,0 +1 @@
+The quit debouncer has been rewritten to be more performant, dropping QUITs entirely until the bridge is able to cope with them.

src/bridge/IrcBridge.ts

@@ -410,8 +410,6 @@ export class IrcBridge {
 
         await this.dataStore.removeConfigMappings();
 
-        this.clientPool = new ClientPool(this, this.dataStore);
-
         if (this.config.ircService.debugApi.enabled) {
             this.debugApi = new DebugApi(
                 this,
@@ -454,6 +452,8 @@ export class IrcBridge {
             this.ircServers.push(server);
         }
 
+        this.clientPool = new ClientPool(this, this.dataStore);
+
         if (this.ircServers.length === 0) {
             throw Error("No IRC servers specified.");
         }

src/bridge/IrcHandler.ts

@@ -1,5 +1,4 @@
 import { IrcBridge } from "./IrcBridge";
-import { QuitDebouncer } from "./QuitDebouncer";
 import { Queue } from "../util/Queue";
 import { RoomAccessSyncer } from "./RoomAccessSyncer";
 import { IrcServer, MembershipSyncKind } from "../irc/IrcServer";
@@ -48,8 +47,6 @@ export class IrcHandler {
     // need to re-invite them if they bail.
     private readonly roomIdToPrivateMember: RoomIdtoPrivateMember = {};
 
-    private readonly quitDebouncer: QuitDebouncer;
-
     // Use per-channel queues to keep the setting of topics in rooms atomic in
     // order to prevent races involving several topics being received from IRC
     // in quick succession. If `(server, channel, topic)` are the same, an
@@ -85,7 +82,6 @@ export class IrcHandler {
         private readonly ircBridge: IrcBridge,
         config: IrcHandlerConfig = {},
         private readonly membershipQueue: MembershipQueue) {
-        this.quitDebouncer = new QuitDebouncer(ircBridge);
         this.roomAccessSyncer = new RoomAccessSyncer(ircBridge);
         this.mentionMode = config.mapIrcMentionsToMatrix || "on";
         this.getMetrics();
@@ -613,8 +609,6 @@ export class IrcHandler {
             return BridgeRequestErr.ERR_VIRTUAL_USER;
         }
 
-        this.quitDebouncer.onJoin(nick, server);
-
         // get virtual matrix user
         const matrixUser = await this.ircBridge.getMatrixUser(joiningUser);
         const matrixRooms = await this.ircBridge.getStore().getMatrixRoomsForChannel(server, chan);
@@ -782,18 +776,6 @@ export class IrcHandler {
         }
         else {
             const matrixUser = await this.ircBridge.getMatrixUser(leavingUser);
-            // Presence syncing and Quit Debouncing
-            //  When an IRC user quits, debounce before leaving them from matrix rooms. In the meantime,
-            //  update presence to "offline". If the user rejoins a channel before timeout, do not part
-            //  user from the room. Otherwise timeout and leave rooms.
-            if (kind === "quit" && server.shouldDebounceQuits()) {
-                const shouldBridgePart = await this.quitDebouncer.debounceQuit(
-                    req, server, matrixUser, nick
-                );
-                if (!shouldBridgePart) {
-                    return undefined;
-                }
-            }
             userId = matrixUser.userId;
         }
 

src/bridge/QuitDebouncer.ts

@@ -1,38 +1,37 @@
-import Bluebird from "bluebird";
 import { IrcServer } from "../irc/IrcServer";
-import { BridgeRequest } from "../models/BridgeRequest";
-import { MatrixUser } from "matrix-appservice-bridge";
-import { IrcBridge } from "../bridge/IrcBridge";
+import Log from "../logging";
+import { Queue } from "../util/Queue";
+
+const log = Log("QuitDebouncer");
 
-const QUIT_WAIT_DELAY_MS = 100;
 const QUIT_WINDOW_MS = 1000;
-const QUIT_PRESENCE = "offline";
 
 export class QuitDebouncer {
     private debouncerForServer: {
         [domain: string]: {
-            rejoinPromises: {
-                [nick: string]: {
-                    resolve: () => void;
-                };
-            };
             quitTimestampsMs: number[];
+            splitChannelUsers: Map<string, Set<string>>; //"$channel $nick"
         };
     };
 
-    constructor(private ircBridge: IrcBridge) {
+    private quitProcessQueue: Queue<{channel: string; server: IrcServer}>;
+
+    constructor(
+        servers: IrcServer[],
+        private handleQuit: (item: {channel: string; server: IrcServer}) => Promise<void>) {
         // Measure the probability of a net-split having just happened using QUIT frequency.
         // This is to smooth incoming PART spam from IRC clients that suffer from a
         // net-split (or other issues that lead to mass PART-ings)
         this.debouncerForServer = {};
+        this.quitProcessQueue = new Queue(this.handleQuit);
 
         // Keep a track of the times at which debounceQuit was called, and use this to
         // determine the rate at which quits are being received. This can then be used
         // to detect net splits.
-        Object.keys(this.ircBridge.config.ircService.servers).forEach((domain) => {
+        Object.values(servers).forEach(({domain}) => {
             this.debouncerForServer[domain] = {
-                rejoinPromises: {},
-                quitTimestampsMs: []
+                quitTimestampsMs: [],
+                splitChannelUsers: new Map(),
             };
         });
     }
@@ -43,14 +42,32 @@ export class QuitDebouncer {
      * @param {string} nick The nick of the IRC user joining.
      * @param {IrcServer} server The sending IRC server.
      */
-    public onJoin(nick: string, server: IrcServer) {
+    public onJoin(nick: string, channel: string, server: IrcServer) {
         if (!this.debouncerForServer[server.domain]) {
             return;
         }
-        const rejoin = this.debouncerForServer[server.domain].rejoinPromises[nick];
-        if (rejoin) {
-            rejoin.resolve();
+        const set = this.debouncerForServer[server.domain].splitChannelUsers.get(channel);
+        if (!set) {
+            return;
+        }
+        set.delete(nick);
+
+        if (set.size === 0) {
+            return;
         }
+        this.quitProcessQueue.enqueue(channel+server.domain, {channel, server});
+    }
+
+    /**
+     * Get a list of nicknames that have been QUIT from a channel.
+     * @param channel The IRC channel
+     * @param server The IRC server
+     */
+    public getQuitNicksForChannel(channel: string, server: IrcServer) {
+        // A little hint on iterators here:
+        // You can return values() (an IterableIterator<string>) and if the Set gets modified,
+        // the iterator will skip the value that was deleted.
+        return this.debouncerForServer[server.domain].splitChannelUsers.get(channel)?.values() || [];
     }
 
     /**
@@ -62,7 +79,10 @@ export class QuitDebouncer {
      * @param {string} nick The nick of the IRC user quiting.
      * @return {Promise} which resolves to true if a leave should be sent, false otherwise.
      */
-    public async debounceQuit (req: BridgeRequest, server: IrcServer, matrixUser: MatrixUser, nick: string) {
+    public debounceQuit (nick: string, server: IrcServer, channels: string[]): boolean {
+        if (!server.shouldDebounceQuits()) {
+            return true;
+        }
         // Maintain the last windowMs worth of timestamps corresponding with calls to this function.
         const debouncer = this.debouncerForServer[server.domain];
 
@@ -77,22 +97,7 @@ export class QuitDebouncer {
         );
 
         // Wait for a short time to allow other potential splitters to send QUITs
-        await Bluebird.delay(QUIT_WAIT_DELAY_MS);
-        const isSplitOccuring = debouncer.quitTimestampsMs.length > threshold;
-
-        // TODO: This should be replaced with "disconnected" as per matrix-appservice-irc#222
-        try {
-            await this.ircBridge.getAppServiceBridge().getIntent(
-                matrixUser.getId()
-            ).setPresence(QUIT_PRESENCE);
-        }
-        catch (err) {
-            req.log.error(
-                `QuitDebouncer Failed to set presence to ${QUIT_PRESENCE} for user %s: %s`,
-                matrixUser.getId(),
-                err.message
-            );
-        }
+        const isSplitOccuring = true || debouncer.quitTimestampsMs.length > threshold;
 
         // Bridge QUITs if a net split is not occurring. This is in the case where a QUIT is
         // received for reasons such as ping timeout or IRC client (G)UI being killed.
@@ -102,32 +107,15 @@ export class QuitDebouncer {
             return true;
         }
 
-        const debounceDelayMinMs = server.getQuitDebounceDelayMinMs();
-        const debounceDelayMaxMs = server.getQuitDebounceDelayMaxMs();
-
-        const debounceMs = debounceDelayMinMs + Math.random() * (
-            debounceDelayMaxMs - debounceDelayMinMs
-        );
-
-        // We do want to immediately bridge a leave if <= 0
-        if (debounceMs <= 0) {
-            return true;
-        }
-
-        req.log.info('Debouncing for ' + debounceMs + 'ms');
-        const promise = new Bluebird((resolve) => {
-            debouncer.rejoinPromises[nick] = {resolve};
-        }).timeout(debounceMs);
-
-        // Return whether the part should be bridged as a leave
-        try {
-            await promise;
-            // User has joined a channel, presence has been set to online, do not leave rooms
-            return false;
-        }
-        catch (err) {
-            req.log.info("User did not rejoin (%s)", err.message);
-            return true;
-        }
+        log.debug(`Dropping QUIT for ${nick}`);
+        channels.forEach((channel) => {
+            if (!debouncer.splitChannelUsers.has(channel)) {
+                debouncer.splitChannelUsers.set(channel, new Set());
+            }
+            // We've already checked above.
+            // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+            debouncer.splitChannelUsers.get(channel)!.add(nick);
+        })
+        return false;
     }
 }

src/irc/ClientPool.ts

@@ -64,6 +64,7 @@ export class ClientPool {
             this.ircBridge.getAppServiceBridge(),
             this,
             this.ircBridge.ircHandler,
+            this.ircBridge.getServers(),
         );
     }
 

src/irc/IrcEventBroker.ts

@@ -91,6 +91,8 @@ import { ClientPool } from "./ClientPool";
 import { BridgedClient, BridgedClientStatus } from "./BridgedClient";
 import { IrcMessage, ConnectionInstance } from "./ConnectionInstance";
 import { IrcHandler } from "../bridge/IrcHandler";
+import { QuitDebouncer } from "../bridge/QuitDebouncer";
+import { IrcServer } from "./IrcServer";
 
 const log = getLogger("IrcEventBroker");
 
@@ -107,12 +109,15 @@ function complete(req: BridgeRequest, promise: Promise<BridgeRequestErr|void>) {
 export class IrcEventBroker {
     private processed: ProcessedDict;
     private channelReqBuffer: {[channel: string]: Promise<unknown>} = {};
+    private quitDebouncer: QuitDebouncer;
     constructor(
         private readonly appServiceBridge: Bridge,
         private readonly pool: ClientPool,
-        private readonly ircHandler: IrcHandler) {
+        private readonly ircHandler: IrcHandler,
+        servers: IrcServer[]) {
         this.processed = new ProcessedDict();
         this.processed.startCleaner(log);
+        this.quitDebouncer = new QuitDebouncer(servers, this.handleDebouncedQuit.bind(this));
     }
 
 
@@ -202,6 +207,37 @@ export class IrcEventBroker {
         });
     }
 
+    /**
+     * This function is called when the quit debouncer has deemed it safe to start sending
+     * quits from users who were debounced.
+     * @param item The channel/server pair to send QUITs from
+     */
+    private async handleDebouncedQuit(item: {channel: string; server: IrcServer}) {
+        const createUser = (nick: string) => {
+            return new IrcUser(
+                item.server, nick,
+                this.pool.nickIsVirtual(item.server, nick)
+            );
+        };
+
+        const createRequest = () => {
+            return new BridgeRequest(
+                this.appServiceBridge.getRequestFactory().newRequest({
+                    data: {
+                        isFromIrc: true
+                    }
+                })
+            );
+        };
+        const req = createRequest();
+        log.info(`Sending delayed QUITs for ${item.channel}`);
+        for (const nick of this.quitDebouncer.getQuitNicksForChannel(item.channel, item.server)) {
+            await complete(req, this.ircHandler.onPart(
+                req, item.server, createUser(nick), item.channel, "quit"
+            ));
+        }
+    }
+
     public sendMetadata(client: BridgedClient, msg: string, force = false, err?: IrcMessage) {
         if ((client.isBot || !client.server.shouldSendConnectionNotices()) && !force) {
             return;
@@ -302,12 +338,15 @@ export class IrcEventBroker {
         });
         this.hookIfClaimed(client, connInst, "quit", (nick: string, reason: string, chans: string[]) => {
             chans = chans || [];
-            chans.forEach((chan) => {
-                const req = createRequest();
-                complete(req, ircHandler.onPart(
-                    req, server, createUser(nick), chan, "quit"
-                ));
-            });
+            // True if a leave should be sent, otherwise false.
+            if (this.quitDebouncer.debounceQuit(nick, server, chans)) {
+                chans.forEach((chan) => {
+                    const req = createRequest();
+                    complete(req, ircHandler.onPart(
+                        req, server, createUser(nick), chan, "quit"
+                    ));
+                });
+            }
         });
         this.hookIfClaimed(client, connInst, "kick", (chan: string, nick: string, by: string, reason: string) => {
             const req = createRequest();
@@ -317,6 +356,7 @@ export class IrcEventBroker {
         });
         this.hookIfClaimed(client, connInst, "join", (chan: string, nick: string) => {
             const req = createRequest();
+            this.quitDebouncer.onJoin(nick, chan, server);
             complete(req, ircHandler.onJoin(
                 req, server, createUser(nick), chan, "join"
             ));