Synchronous Busy-Wait Loop + Multiple Missing await Bugs in EmbeddedChatApi.ts #1166

Bug: Synchronous busy-wait loop and missing await calls in EmbeddedChatApi.ts

Description

packages/api/src/EmbeddedChatApi.ts contains a synchronous busy-wait anti-pattern in the typing event handler and several missing await keywords that bypass error handling in async API methods.

1. Synchronous busy-wait loop in handleTypingEvent (L354–L381)

handleTypingEvent({ typingUser, isTyping }) {
    // don't wait for more than 2 seconds.
    setTimeout(() => {
      typingHandlerLock = 0;
    }, 2000);
    // eslint-disable-next-line no-empty
    while (typingHandlerLock) {}   // <-- busy-wait blocks the event loop
    typingHandlerLock = 1;
    // move user to front if typing else remove it.
    const idx = this.typingUsers.indexOf(typingUser);
    if (idx !== -1) {
      this.typingUsers.splice(idx, 1);
    }
    if (isTyping) {
      this.typingUsers.unshift(typingUser);
    }
    typingHandlerLock = 0;
    const newTypingStatus = cloneArray(this.typingUsers);
    this.onTypingStatusCallbacks.forEach((callback) =>
      callback(newTypingStatus)
    );
}

This attempts to implement a mutex using a synchronous while loop. This is fundamentally wrong in JavaScript:

• The lock is unnecessary. JavaScript is single-threaded. WebSocket callbacks already execute atomically — there is no concurrent access to typingUsers, so no lock is needed.
• If the lock is ever stuck at 1 (e.g. an exception thrown between L367–L376 before the reset on L376), the while loop becomes an infinite loop that freezes the browser tab.
• The setTimeout safety net on L362 cannot rescue it. While the while loop is spinning synchronously, the event loop is blocked, so the setTimeout callback sitting in the task queue will never execute.
• The // eslint-disable-next-line no-empty comment shows the linter flagged this and it was suppressed rather than fixed.

2. Missing await on fetch() in sendAttachment (L1068)

async sendAttachment(file, fileName, fileDescription = "", threadId = undefined) {
    try {
      // ...
      const response = fetch(`${this.host}/api/v1/rooms.upload/${this.rid}`, {
        method: "POST",
        body: form,
        headers: { "X-Auth-Token": authToken, "X-User-Id": userId },
      }).then((r) => r.json());
      return response;
    } catch (err) {
      console.log(err);   // <-- this never catches fetch errors
    }
}

Because fetch() is not awaited, the Promise is returned before it settles. This means the try/catch block is completely bypassed for any fetch error — network failures, auth errors, server errors all become unhandled Promise rejections instead of being caught. The console.log(err) on L1078 is dead code for fetch-related errors.

Caller impact (AttachmentPreview.js L55–68):

const submit = async () => {
    setIsPending(true);
    await RCInstance.sendAttachment(data, fileName, ...);
    toggle();              // runs even if upload failed
    setData(null);         // clears data even if upload failed
};

The caller has no try/catch either. When a network error occurs, the unhandled rejection crashes silently — no error message is shown to the user, but the attachment preview closes as if the upload succeeded.

Proof

A minimal reproduction script (proof.js) demonstrates this using real fetch() against an unreachable host:

// Mirrors EmbeddedChatApi.ts L1068 — fetch() without await
async function sendAttachment_current(host) {
  try {
    const response = fetch(`${host}/api/v1/rooms.upload/test`, {
      method: "POST",
    }).then((r) => r.json());
    return response;
  } catch (err) {
    console.log("  [INTERNAL try/catch] Caught:", err.message);
    return "error was handled internally";
  }
}

// Same code with await added
async function sendAttachment_fixed(host) {
  try {
    const response = await fetch(`${host}/api/v1/rooms.upload/test`, {
      method: "POST",
    }).then((r) => r.json());
    return response;
  } catch (err) {
    console.log("  [INTERNAL try/catch] Caught:", err.message);
    return "error was handled internally";
  }
}

Output (node proof.js):

=== CURRENT CODE (no await — from EmbeddedChatApi.ts L1068) ===

  ERROR ESCAPED past try/catch to caller: fetch failed

  The try/catch inside sendAttachment NEVER ran.
  In the real app (AttachmentPreview.js L55-68), the caller
  has no try/catch either, so this becomes an unhandled rejection.


=== FIXED CODE (with await added) ===

  [INTERNAL try/catch] Caught: fetch failed
  Result: error was handled internally

The current code's try/catch is completely bypassed. Adding await makes it work as intended.

3. Missing await on response.json() in three methods (L1246, L1263, L1280)

// getUserStatus (L1246)
const data = response.json();   // missing await
return data;

// userInfo (L1263)
const data = response.json();   // missing await
return data;

// userData (L1280)
const data = response.json();   // missing await
return data;

Note: Because these are async functions and callers use await, JavaScript's Promise auto-flattening means the data resolves correctly in the happy path. However, the missing await means any .json() parse error (e.g. a non-JSON response from the server) would not be catchable within these methods — the error propagates as an unhandled rejection instead of being handled locally.

This is also inconsistent with the rest of the file — e.g. getCommandsList on L1229 correctly uses const data = await response.json().

Impact summary

Steps to reproduce

sendAttachment error-handling bypass (proof script)

No server needed. Run the included proof script:

node proof.js

This uses real fetch() against an unreachable host (localhost:1) and shows that the current code's try/catch never fires, while the fixed version catches the error correctly.

sendAttachment in the live app

1. Go to https://rocketchat.github.io/EmbeddedChat/
2. Log in with username test_acc, password test_acc
3. Open browser DevTools (F12) → Console tab
4. Open DevTools → Network tab → set throttling to Offline
5. Click the attachment/file-upload button in the chat input
6. Select any file and confirm the upload
7. Observe: the attachment preview closes as if the upload succeeded, but the console shows an unhandled Promise rejection (not a caught error)

Busy-wait loop (code inspection)

Read packages/api/src/EmbeddedChatApi.ts line 366 — the while (typingHandlerLock) {} with a suppressed ESLint warning on the line above. The lock is unnecessary in single-threaded JavaScript.

Suggested fix

1. Remove the busy-wait entirely. The lock is unnecessary in single-threaded JS — array operations on typingUsers are already atomic. Remove typingHandlerLock, the while loop, and the setTimeout.
2. Add await before fetch() in sendAttachment so the try/catch actually catches network/server errors.
3. Add await before response.json() in getUserStatus, userInfo, and userData for consistency and proper error handling — matching the pattern used elsewhere in the file (e.g. getCommandsList on L1229).