From e6cf770c90c918a6dcefc019aa31a521225bd7ba Mon Sep 17 00:00:00 2001
From: Greg Price <greg@zulipchat.com>
Date: Fri, 6 Nov 2020 15:04:31 -0800
Subject: [PATCH] outbox [nfc]: Make explicit the odd "async" loop in
 trySendMessages.

This loop doesn't do what it looks like it does.  It looks like a
loop that tries to send one message, awaits that, then tries the
next, and so on.  In fact, because Array#forEach does nothing with
the return values of its callback -- in particular it doesn't notice
if they're promises or anything else, and doesn't await them -- the
actual behavior is to fire off a bunch of requests in parallel, wait
for nothing, and ignore errors.

That behavior isn't good, and we should fix it; that's #3881.

For a start, just make the code more explicit about what it actually
does: desugar the async/await keywords to the equivalent use of
Promise#then, and add a comment for good measure.

This also smooths the way to rewriting Array#forEach here as part of
an automated sweep across our code.
---
 src/outbox/outboxActions.js | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js
index 352236ac81..e4a962216a 100644
--- a/src/outbox/outboxActions.js
+++ b/src/outbox/outboxActions.js
@@ -45,13 +45,16 @@ export const messageSendComplete = (localMessageId: number): Action => ({
   local_message_id: localMessageId,
 });
 
+// TODO: (filed as #3881) The try/catch in here doesn't apply to the actual
+//   sending.  As a result, this will return true even when we're unable to
+//   reach the server, and so the caller won't retry.
 export const trySendMessages = (dispatch: Dispatch, getState: GetState): boolean => {
   const state = getState();
   const auth = getAuth(state);
   const outboxToSend = state.outbox.filter(outbox => !outbox.isSent);
   const oneWeekAgoTimestamp = Date.now() / 1000 - 60 * 60 * 24 * 7;
   try {
-    outboxToSend.forEach(async item => {
+    outboxToSend.forEach(item => {
       // If a message has spent over a week in the outbox, it's probably too
       // stale to try sending it.
       //
@@ -60,7 +63,7 @@ export const trySendMessages = (dispatch: Dispatch, getState: GetState): boolean
       // that instead.
       if (item.timestamp < oneWeekAgoTimestamp) {
         dispatch(deleteOutboxMessage(item.id));
-        return; // i.e., continue
+        return;
       }
 
       // prettier-ignore
@@ -74,15 +77,18 @@ export const trySendMessages = (dispatch: Dispatch, getState: GetState): boolean
             //   CSV, then a literal. To avoid misparsing, always use JSON.
           : JSON.stringify([item.display_recipient]);
 
-      await api.sendMessage(auth, {
+      // Prettier does something silly with this method chaining.
+      // prettier-ignore
+      api.sendMessage(auth, {
         type: item.type,
         to,
         subject: item.subject,
         content: item.markdownContent,
         localId: item.timestamp,
         eventQueueId: state.session.eventQueueId,
+      }).then(() => {
+        dispatch(messageSendComplete(item.timestamp));
       });
-      dispatch(messageSendComplete(item.timestamp));
     });
     return true;
   } catch (e) {