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

[AbhiramVSA]

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

Bug	Affected feature	Impact
Busy-wait loop	Typing indicators	Unnecessary lock in single-threaded JS; potential infinite loop/tab freeze if lock gets stuck
Missing await in sendAttachment	File uploads	try/catch bypassed — fetch errors become unhandled rejections; UI closes as if upload succeeded
Missing await in getUserStatus	User presence	Inconsistent error handling; .json() parse errors become unhandled rejections
Missing await in userInfo	User profiles	Same as above
Missing await in userData	User lookups	Same as above

---

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).

[AbhiramVSA]

Opened a fix for this: #1167

[Utkarsh2294]

Hi! I went through this issue and I understand all three bugs.
First, the busy-wait loop in handleTypingEvent. The while loop with typingMentorLock blocks the JS event loop permanently. Since JavaScript is single-threaded, the lock can never be released once it is set. The fix is to just remove the lock entirely as no synchronization is needed here.

Second, the missing await on fetch() in sendAttachment at L1068. Without await the try/catch block is completely bypassed and any network error silently becomes an unhandled rejection instead of being caught. Adding await before the fetch call fixes this.

Third, the missing await on response.json() at L1246, L1261 and L1280. Same kind of issue where data ends up holding a Promise object instead of the actual parsed JSON which causes silent failures. Adding await in all three places resolves it.

I would love to work on this. Can this be assigned to me?

[AbhiramVSA]

@Utkarsh2294 Hey, I've already opened a PR. Would be nice if you can review that instead.