Skip to content

Commit

Permalink
outbox [nfc]: Make explicit the odd "async" loop in trySendMessages.
Browse files Browse the repository at this point in the history
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 zulip#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.
  • Loading branch information
gnprice committed Nov 11, 2020
1 parent 0efafc6 commit c20da0e
Showing 1 changed file with 10 additions and 4 deletions.
14 changes: 10 additions & 4 deletions src/outbox/outboxActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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
Expand All @@ -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) {
Expand Down

0 comments on commit c20da0e

Please sign in to comment.