fix: replace all async foreach callbacks by nicktrn · Pull Request #10157 · calcom/cal.com

nicktrn · 2023-07-14T11:35:33Z

What does this PR do?

forEach(asyncFn) => await Promise.all(map(asyncFn))
some error logging and refactoring
add eslint rule to prevent reoccurence

Fixes #10156
/claim #10157

Also fixes:

flaky tests (possibly)
longterm outstanding issues (likely)
code quality (definitely)
tears, sweat, and banging your head against the wall (definitely)

Type of change

Bug fix (non-breaking change which fixes an issue)
Chore (refactoring code, technical debt, workflow improvements)

Caveat

Since we wait for promises to resolve, this will add parent function execution time - hopefully not much. In any case, this is likely preferable to unpredictable results.

Todo

add eslint rule to prevent reoccurence
fix similar issues
- async function calls without await - there are many
- implicit async .forEach with promise return
rfc: consider additional eslint rules
- no-async-promise-executor
- no-await-in-loop
- no-return-await
- require-await
rfc: task queues with error handling, concurrency limits, timeout, retry, rollback, etc
rfc: use queues to add real background task runners
rfc: standardise error logging / compliance

Mandatory Tasks

Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

I haven't added tests that prove my fix is effective or that my feature works
I haven't checked if new and existing unit tests pass locally with my changes

vercel · 2023-07-14T11:35:38Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
ui	❌ Failed (Inspect)			Aug 26, 2023 0:38am

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)	Visit Preview		Aug 26, 2023 0:38am

vercel · 2023-07-14T11:35:40Z

@nicktrn is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

github-actions · 2023-07-14T11:35:45Z

Thank you for following the naming conventions! 🙏

github-actions · 2023-07-14T11:43:17Z

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

nicktrn · 2023-07-14T11:38:17Z

packages/app-store/vital/lib/reschedule.ts

    );
+
+    const promises = bookingRefsFiltered.map(async (bookingRef) => {
+      if (!bookingRef.uid) return;


early return is preferable

nicktrn · 2023-07-14T11:40:23Z

packages/app-store/vital/lib/reschedule.ts

-      });
+      await Promise.all(promises);
    } catch (error) {
+      // FIXME: error logging - non-Error type errors are currently discarded


probably just a way around ts complaints, Promise.all will always throw instanceof Error

nicktrn · 2023-07-14T11:41:38Z

packages/app-store/zapier/lib/nodeScheduler.ts

 ) {
  try {
    //schedule job to call subscriber url at the end of meeting
+    // FIXME: in-process scheduling - job will vanish on server crash / restart


just for future reference

nicktrn · 2023-07-14T11:42:53Z

packages/app-store/zapier/lib/nodeScheduler.ts

  appId?: string | null,
  isReschedule?: boolean
 ) {
-  try {


less nesting

nicktrn · 2023-07-14T11:45:30Z

packages/eslint-plugin/src/rules/no-async-foreach-callback.ts

+        if (
+          callee.type === "MemberExpression" &&
+          callee.property.type === "Identifier" &&
+          callee.property.name === "forEach"


There is no type check here, so other non-Array forEach method calls may also be flagged. Should not be an issue for this project.

nicktrn · 2023-07-14T11:55:54Z

packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts

+  const bookingRefsFiltered: BookingReference[] = bookingToReschedule.references.filter((ref) =>
+    credentialsMap.has(ref.type)
+  );
+  const pCancellations = bookingRefsFiltered.map(async (bookingRef) => {


another async forEach callback

nicktrn · 2023-07-14T11:58:05Z

packages/trpc/server/routers/viewer/workflows/update.handler.ts

-      steps.forEach(async (step) => {
-        if (step.action !== WorkflowActions.SMS_ATTENDEE && step.action !== WorkflowActions.WHATSAPP_ATTENDEE) {
+      const pSteps = steps.map(async (step) => {
+        if (
+          step.action !== WorkflowActions.SMS_ATTENDEE &&
+          step.action !== WorkflowActions.WHATSAPP_ATTENDEE
+        ) {
          //as we do not have attendees phone number (user is notified about that when setting this action)
-          bookingsForReminders.forEach(async (booking) => {


nested async forEach, when you think you've seen it all

nicktrn · 2023-07-14T12:00:13Z

packages/trpc/server/routers/viewer/workflows/update.handler.ts

+    await ctx.prisma.workflowsOnEventTypes.createMany({
+      data: activeOnEventTypes.map((eventType) => ({
+        workflowId: id,
+        eventTypeId: eventType.id,
+      })),


this should run a lot quicker - only one query

Let's do it as a separate performance optimization in another PR.
-- Edit
Nevermind, I think it's needed to solve the problem correctly. So, let's keep it.

nicktrn · 2023-07-14T12:01:10Z

packages/trpc/server/routers/viewer/workflows/update.handler.ts

+    const pChildren = activeOnEventTypes.map((eventType) =>
+      ctx.prisma.workflowsOnEventTypes.createMany({
+        data: eventType.children.map((chEventType) => ({
+          workflowId: id,
+          eventTypeId: chEventType.id,
+        })),
+      })
+    );
+    await Promise.all(pChildren);


so should this

nicktrn · 2023-07-14T12:02:11Z

packages/trpc/server/routers/viewer/workflows/update.handler.ts

+        } else if (reminder.method === WorkflowMethods.WHATSAPP) {
+          deleteScheduledWhatsappReminder(reminder.id, reminder.referenceId);


seems this was forgotten here, but similar calls found elsewhere in the file

That's interesting. @CarinaWolli can you confirm that the flow was missing whatsapp here.

@nicktrn it can be a separate PR though to fix it, so that this change doesn't block this PR.

I think there have been quite a few changes here since the last merge, probably fixed now!

Not fixed on main, will revert it for now.

I was also thinking that maybe classes would make sense here. We can have classes for all these channels(Whatsapp, SMS, Email) and the code here would simply do channel.deleteReminder. This would avoid having these if elses everywhere. This code is just too much involved with what's going on.

But classes might increase complexity in some other way.

That's interesting. @CarinaWolli can you confirm that the flow was missing whatsapp here.

Yes, definitely looks like this was missing here. Thanks for finding that 🙏

Back in it goes 😄

nicktrn · 2023-07-17T22:54:10Z

Came across even more instances of this in different forms. It's impossible to tell in many places if this is desired behaviour or not.

Might make sense to be explicit by requiring the use of a wrapper function like this:

type NoArgFunction = () => unknown;

const asyncWrap = async (fn: NoArgFunction) => fn();

export const sendToBackgroundAndNeverThrow = (fn: NoArgFunction | NoArgFunction[]): void => {
  if (typeof fn === "function") {
    Promise.allSettled([asyncWrap(fn)]);
  } else {
    Promise.allSettled(fn.map(asyncWrap));
  }
};

Having to call sendToBackgroundAndNeverThrow(() => someTask(args)) would make the intentions very clear. Those calls can then also be more easily found and migrated whenever background task queues are implemented.

nicktrn · 2023-08-10T00:07:43Z

Closing as this was rather premature, and now outdated.

keithwillcode · 2023-08-24T11:15:36Z

Closing as this was rather premature, and now outdated.

This is a great initiative @nicktrn. Want to pick it back up?

/bounty 50

algora-pbc · 2023-08-24T11:15:39Z

💎 $50 bounty created by keithwillcode
👉 No need to comment asking to work on it. Just open a PR and claim the bounty with /claim #10157 inside the PR
📝 Before proceeding, please make sure you can receive payouts in your country
💵 Payment arrives in your account 2-5 days after the bounty is rewarded
💯 You keep 100% of the bounty award
🙏 Thank you for contributing to calcom/cal.com!

hariombalhara · 2023-08-24T11:27:27Z

packages/trpc/server/routers/viewer/workflows/update.handler.ts

      });

-      steps.forEach(async (step) => {
+      const pSteps = steps.map(async (step) => {


Suggested change

const pSteps = steps.map(async (step) => {

const promiseSteps = steps.map(async (step) => {

Let's be clear here.

nicktrn · 2023-08-24T11:31:49Z

@keithwillcode Sure thing, will try to resolve those merge conflicts first of all. It's been a while.

There are some decisions to be made on how to proceed though:

Should this PR only deal with async forEach or un-awaited promises in general?
I can replace all occurences with await Promise.all() but this could mean adding execution time to very commonly accessed routes. Is this desired?

An alternative could be to add a wrapper as suggested in #10157 (comment) for now, and include error logging.

nicktrn · 2023-08-24T11:34:59Z

@hariombalhara thanks for the review pointers so far, will help to keep this focused.

hariombalhara · 2023-08-24T11:52:06Z

packages/trpc/server/routers/viewer/workflows/update.handler.ts

+          deleteScheduledWhatsappReminder(reminder.id, reminder.referenceId);
        }
      });
+      await Promise.all(pDeleteReminders);


How do you think about wrapping the code itself in Promise.all vs storing the result in pDeleteReminders and then doing await.

I find the earlier approach easier to read.

So this becomes,

await Promise.all(remindersToUpdate.map((reminder) => { if (reminder.method === WorkflowMethods.EMAIL) { deleteScheduledEmailReminder(reminder.id, reminder.referenceId); } else if (reminder.method === WorkflowMethods.SMS) { deleteScheduledSMSReminder(reminder.id, reminder.referenceId); } else if (reminder.method === WorkflowMethods.WHATSAPP) { deleteScheduledWhatsappReminder(reminder.id, reminder.referenceId) } })

Sure that works. For longer mapping functions I tend to prefer prior assignment, but this is rather short.

Will actually revert the whole thing. One of those implicit async calls that could probably do better with Promise.allSettled() in another PR. Error handling should really be discussed.

hariombalhara · 2023-08-24T11:56:29Z

packages/app-store/zapier/lib/nodeScheduler.ts

-  try {
-    let scheduledJobs = booking.scheduledJobs || [];
+  let scheduledJobs = booking.scheduledJobs || [];
+  if (!booking.scheduledJobs) return;


What's the point of fallback initialization with empty array if we are still early returning in that case.

hariombalhara · 2023-08-24T13:46:37Z

@keithwillcode Sure thing, will try to resolve those merge conflicts first of all. It's been a while.

There are some decisions to be made on how to proceed though:

Should this PR only deal with async forEach or un-awaited promises in general?

I can replace all occurences with await Promise.all() but this could mean adding execution time to very commonly accessed routes. Is this desired?

An alternative could be to add a wrapper as suggested in #10157 (comment) for now, and include error logging.

I think we can do it in two parts because unawaited promises in general is difficult to find out without eslint plugin setup(I guess?)
That should be fine IMHO. The alternate approach isn't easy to read and difficult to reason a bit. We should go for that if that really makes sense.

nicktrn · 2023-08-24T14:05:10Z

packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts

-  });
-  await Promise.all(pCancellations);
+  // FIXME: error-handling
+  await Promise.allSettled(


Think this one is better suited for allSettled. Failure to delete a meeting should probably not prevent reschedule.

nicktrn · 2023-08-24T14:28:11Z

I think we can do it in two parts because unawaited promises in general is difficult to find out without eslint plugin setup(I guess?)

That should be fine IMHO. The alternate approach isn't easy to read and difficult to reason a bit. We should go for that if that really makes sense.

ESLint will definitely make this easier!
Have gone with that for now.

Have to say I'm quite worried by this PR. Will definitely need to be followed up with proper error handling / retries, at the very least additional logging.

A big issue is that those async calls are now back in function scope, i.e. any rejected promise will prevent normal return. The good thing is this should highlight previously hidden errors.

Are there any plans for background tasks? Think I saw some mention of redis-backed solutions a while back. Could also consider Trigger.dev.

This reverts commit 53dfbcf.

keithwillcode · 2023-08-25T09:58:11Z

@keithwillcode Sure thing, will try to resolve those merge conflicts first of all. It's been a while.
There are some decisions to be made on how to proceed though:

Should this PR only deal with async forEach or un-awaited promises in general?

I can replace all occurences with await Promise.all() but this could mean adding execution time to very commonly accessed routes. Is this desired?

An alternative could be to add a wrapper as suggested in #10157 (comment) for now, and include error logging.

I think we can do it in two parts because unawaited promises in general is difficult to find out without eslint plugin setup(I guess?)

That should be fine IMHO. The alternate approach isn't easy to read and difficult to reason a bit. We should go for that if that really makes sense.

Agree with this approach 🙏🏼. @nicktrn make sure you claim this PR so you get the bounty pay out when it merges

nicktrn · 2023-08-25T10:11:32Z

Ah yes, thanks @keithwillcode. Appreciate the gesture but really wasn't necessary!

algora-pbc · 2023-08-25T10:11:56Z

💡 @nicktrn submitted a pull request that claims the bounty. You can visit your org dashboard to reward.

zomars

LGTM. Thank you for your contribution 🙏

algora-pbc · 2023-08-26T00:43:27Z

🎉🎈 @nicktrn has been awarded $50! 🎈🎊

github-actions bot added the 🐛 bug Something isn't working label Jul 14, 2023

nicktrn changed the title ~~Fix: WIP: replace all async foreach callbacks~~ fix: WIP: replace all async foreach callbacks Jul 14, 2023

vercel bot deployed to Preview – ui July 14, 2023 11:39 View deployment

nicktrn commented Jul 14, 2023

View reviewed changes

nicktrn changed the title ~~fix: WIP: replace all async foreach callbacks~~ fix: [RFC] replace all async foreach callbacks Jul 17, 2023

This was referenced Jul 21, 2023

[CAL-2109] Reschedule Emails not received by the Scheduler #9957

Closed

[CAL-2263] Add @typescript-eslint/no-floating-promises and https://typescript-eslint.io/rules/no-misused-promises #10398

Closed

nicktrn force-pushed the fix/cq-async-foreach branch from 361d55f to 3f1ad68 Compare July 26, 2023 21:14

vercel bot deployed to Preview – ui July 26, 2023 21:18 View deployment

nicktrn closed this Aug 10, 2023

nicktrn mentioned this pull request Aug 16, 2023

Async forEach callbacks cause unpredictable results and prevent error handling #10156

Closed

hariombalhara reopened this Aug 24, 2023

algora-pbc bot added the 💎 Bounty A bounty on Algora.io label Aug 24, 2023

hariombalhara reviewed Aug 24, 2023

View reviewed changes

nicktrn added 2 commits August 24, 2023 13:22

fix: replace all async forEach calls

33b1249

chore: add no-async-foreach-callback eslint rule

53dfbcf

fix: small rename and refactor

7d2c886

nicktrn force-pushed the fix/cq-async-foreach branch from 3f1ad68 to 7d2c886 Compare August 24, 2023 14:01

vercel bot had a problem deploying to Preview – ui August 24, 2023 14:05 Failure

nicktrn commented Aug 24, 2023

View reviewed changes

Revert "chore: add no-async-foreach-callback eslint rule"

3482ee9

This reverts commit 53dfbcf.

nicktrn requested a review from hariombalhara August 24, 2023 14:34

vercel bot had a problem deploying to Preview – ui August 24, 2023 14:34 Failure

fix: add missing whatsapp reminder delete call

27a8e4a

vercel bot had a problem deploying to Preview – ui August 24, 2023 21:58 Failure

nicktrn marked this pull request as ready for review August 24, 2023 23:53

nicktrn changed the title ~~fix: [RFC] replace all async foreach callbacks~~ fix: replace all async foreach callbacks Aug 24, 2023

zomars previously approved these changes Aug 26, 2023

View reviewed changes

zomars added 2 commits August 25, 2023 17:29

Merge branch 'main' into pr/10157

1f6ee8a

Merge branch 'main' into fix/cq-async-foreach

6ebdfe9

zomars dismissed their stale review via 6ebdfe9 August 26, 2023 00:31

zomars approved these changes Aug 26, 2023

View reviewed changes

zomars enabled auto-merge (squash) August 26, 2023 00:34

vercel bot had a problem deploying to Preview – ui August 26, 2023 00:38 Failure

zomars merged commit c1bcfcf into calcom:main Aug 26, 2023

algora-pbc bot added the 💰 Rewarded Rewarded bounties on Algora.io label Aug 26, 2023

		} else if (reminder.method === WorkflowMethods.WHATSAPP) {
		deleteScheduledWhatsappReminder(reminder.id, reminder.referenceId);

	const pSteps = steps.map(async (step) => {
	const promiseSteps = steps.map(async (step) => {

Conversation

nicktrn commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

Caveat

Todo

Mandatory Tasks

Checklist

Uh oh!

vercel bot commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jul 14, 2023

Uh oh!

github-actions bot commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hariombalhara Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicktrn commented Jul 17, 2023

Uh oh!

nicktrn commented Aug 10, 2023

Uh oh!

keithwillcode commented Aug 24, 2023

Uh oh!

algora-pbc bot commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicktrn commented Aug 24, 2023

Uh oh!

nicktrn commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hariombalhara Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

nicktrn commented Jul 14, 2023 •

edited

Loading

vercel bot commented Jul 14, 2023 •

edited

Loading

github-actions bot commented Jul 14, 2023 •

edited

Loading

github-actions bot commented Jul 14, 2023 •

edited

Loading

hariombalhara Aug 24, 2023 •

edited

Loading

algora-pbc bot commented Aug 24, 2023 •

edited

Loading

hariombalhara Aug 24, 2023 •

edited

Loading