From 25ad99637d3b5b5612173a8a80e17cf52e82fc5c Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 29 Dec 2022 00:44:31 +0000
Subject: [PATCH] FIX: Avoid race conditions when toggling presence state
 (#19648)

We need to set the local state of a channel before performing any async operations. Otherwise, multiple leave/join calls can race against each other and cause the local state to get out-of-sync with the server.

Followup to e70ed31a
---
 .../discourse/app/services/presence.js        |  4 +-
 .../tests/unit/services/presence-test.js      | 54 +++++++++++++++++--
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/services/presence.js b/app/assets/javascripts/discourse/app/services/presence.js
index 91becb949c6..8949b99ee47 100644
--- a/app/assets/javascripts/discourse/app/services/presence.js
+++ b/app/assets/javascripts/discourse/app/services/presence.js
@@ -67,16 +67,16 @@ class PresenceChannel extends EmberObject.extend(Evented) {
 
     this.setProperties({ activeOptions });
     if (!this.present) {
-      await this.presenceService._enter(this);
       this.set("present", true);
+      await this.presenceService._enter(this);
     }
   }
 
   // Mark the current user as leaving this channel
   async leave() {
     if (this.present) {
-      await this.presenceService._leave(this);
       this.set("present", false);
+      await this.presenceService._leave(this);
     }
   }
 
diff --git a/app/assets/javascripts/discourse/tests/unit/services/presence-test.js b/app/assets/javascripts/discourse/tests/unit/services/presence-test.js
index 1b332e0524d..3f5d1d09dd9 100644
--- a/app/assets/javascripts/discourse/tests/unit/services/presence-test.js
+++ b/app/assets/javascripts/discourse/tests/unit/services/presence-test.js
@@ -32,7 +32,7 @@ function usersFixture() {
 
 acceptance("Presence - Subscribing", function (needs) {
   needs.pretender((server, helper) => {
-    server.get("/presence/get", (request) => {
+    server.get("/presence/get", async (request) => {
       const channels = request.queryParams.channels;
       const response = {};
 
@@ -245,10 +245,29 @@ acceptance("Presence - Subscribing", function (needs) {
 acceptance("Presence - Entering and Leaving", function (needs) {
   needs.user();
 
+  let responseStartPromise;
+  let resolveResponseStartPromise;
+  let responseWaitPromise;
+
   const requests = [];
-  needs.hooks.afterEach(() => requests.clear());
+
+  needs.hooks.beforeEach(() => {
+    responseStartPromise = new Promise(
+      (resolve) => (resolveResponseStartPromise = resolve)
+    );
+  });
+
+  needs.hooks.afterEach(() => {
+    requests.clear();
+    responseWaitPromise = null;
+    responseStartPromise = null;
+  });
+
   needs.pretender((server, helper) => {
-    server.post("/presence/update", (request) => {
+    server.post("/presence/update", async (request) => {
+      resolveResponseStartPromise();
+      await responseWaitPromise;
+
       const body = new URLSearchParams(request.requestBody);
       requests.push(body);
 
@@ -325,6 +344,35 @@ acceptance("Presence - Entering and Leaving", function (needs) {
     );
   });
 
+  test("interleaved join/leave calls work correctly", async function (assert) {
+    let resolveServerResponse;
+    responseWaitPromise = new Promise(
+      (resolve) => (resolveServerResponse = resolve)
+    );
+
+    const presenceService = this.container.lookup("service:presence");
+    const channel = presenceService.getChannel("/test/ch1");
+
+    const enterPromise = channel.enter();
+
+    await responseStartPromise;
+
+    const leavePromise = channel.leave();
+
+    resolveServerResponse();
+
+    await enterPromise;
+    await leavePromise;
+
+    assert.strictEqual(requests.length, 2, "server received two requests");
+
+    const presentChannels = requests.map((r) => r.getAll("present_channels[]"));
+    const leaveChannels = requests.map((r) => r.getAll("leave_channels[]"));
+
+    assert.deepEqual(presentChannels, [["/test/ch1"], []]);
+    assert.deepEqual(leaveChannels, [[], ["/test/ch1"]]);
+  });
+
   test("raises an error when entering a non-existent channel", async function (assert) {
     const presenceService = this.container.lookup("service:presence");
     const channel = presenceService.getChannel("/blah/does-not-exist");