[eslint] add rule to forbid async forEach bodies by spalger · Pull Request #111637 · elastic/kibana

spalger · 2021-09-08T19:35:55Z

I noticed an unresolved promise rejection on CI and tracked it back to a usage of array.forEach() which was passed an async function. Like #110349 this creates hidden promises which can't be tracked, preventing the rejections from being handled. To prevent this mistake I've implemented an ESLint rule to prevent devs from accidentally using this pattern.

Unlike in the other PR I can't think of a lowest-common-denominator fix to implement across the board, so I have instead fixed all the violations manually. The fixes use one of three new helpers added to @kbn/std:

asyncForEach(iter, fn): The most common replacement was to just replace array.forEach() with a helper that does the same thing. This calls fn synchronously for each item in array and waits for the returned promise/observables to resolve/reject/complete/error.
asyncMap(iter, fn): A common usage of async forEach() was to add items to an array within scope. I have replaced those with asyncMap() which calls fn synchronously for each item in array and waits for the returned promise/observables to resolve/reject/complete/error. The result array has each item in the order of the source array.
map$(iter, fn): A simple wrapper around RxJS for creating a stream from an iterable value where each item in the result is the return value of calling the map function with each item from the iterable. This is the basis of the other functions.
No need for async: There were a handful of places where the function passed to .forEach() was async unnecessarily.
Use a library: In a few places we could just replace the existing logic with a dependency which is already available and does the intended logic without any compromises.

Each helper implemented in this PR includes a variant which accepts a limit parameter between the iterable and the map function. I'm interested in having a discussion about if we should be using these instead of starting each concurrent iteration at the same time, potentially creating a lot more load from a single request on the server, or doing a lot more work on each frame in the UI. It's also possible that several places where I updated code take input from users which are variable in length and could be very long. In those cases I think it would make sense to limit the amount of work we are doing concurrently... idk, I originally wrote the "unlimited" helpers to have a default concurrency of 4 but got sheepish. For now the *WithLimit() versions are unused outside of the unlimited helpers, so if reviewers request I will remove them.

I could also imagine us requiring usage of such helpers instead of doing Promise.all(array.map()) which we do in many, many places. It could still be possible with something like asyncMapWithLimit(iter, Infinity, fn) but would require opting into this when the dev knows it won't lead to too much concurrency.

...ns/security/public/management/users/components/confirm_delete_users/confirm_delete_users.tsx

src/plugins/vis_type_timelion/server/lib/reduce.js

src/plugins/bfetch/server/plugin.ts

…ion helpers

…int-no-async-foreach

mshustov · 2021-09-13T16:52:07Z

packages/elastic-eslint-config-kibana/.eslintrc.js

    ],

    '@kbn/eslint/no_async_promise_body': 'error',
+    '@kbn/eslint/no_async_foreach': 'error',


I believe we should expand the rule to cover other expressions, like this one #98463 (comment)
I'm not sure whether no-floating-promises implements the same check, but we can give it a try.

Yeah, I'll look at a similar solution for that. I don't want to overload this rule though and have a special error message for that scenario.

I don't want to overload this rule though and have a special error message for that scenario.

Would you suggest implementing a custom rule for every use-case when (...args: any[]) => Promise<any> is used instead of (...args: any[]) => void?

I do think that having custom error messages for each situation would be nice, though I'm not convinced there are that many scenarios right now. It's possibly a bigger more wide issue than I'm thinking though.

mshustov · 2021-09-13T16:55:25Z

packages/kbn-eslint-plugin-eslint/rules/no_async_foreach.js

+  return (
+    node.callee.type === esTypes.MemberExpression &&
+    node.callee.property.type === esTypes.Identifier &&
+    node.callee.property.name === 'forEach' &&


unfortunately, it doesn't cover cases when a promise is returned without an async keyword 😞

Yeah, in that case we're creating a promise which can be handled, so I feel like it's less of an issue.

Yeah, in that case we're creating a promise which can be handled

Could you elaborate on it? Sorry, I'm not follow who handles a promise in this case 😞

async function doAsync () {} [].forEach(doAsync)

Ah, I figured you were describing:

array.forEach(() => new Promise((resolve) => ...))

There's a chance we could improve this to use types for locally defined functions, but the more research I do on other things the more I feel like we need to start providing ESLint with full types... Which has a lot of complications.

packages/kbn-std/src/iteration/test_helpers.ts

packages/kbn-std/src/iteration/for_each.test.ts

packages/kbn-std/src/iteration/test_helpers.ts

packages/kbn-std/src/iteration/for_each.ts

packages/kbn-std/src/index.ts

…int-no-async-foreach

YulNaumenko

Alerting changes LGTM!

elasticmachine · 2021-09-13T23:41:25Z

Pinging @elastic/fleet (Team:Fleet)

jportner

Security changes LGTM!

flash1293

The timelion one scares me a bit, but I think this comment here

kibana/src/plugins/vis_type_timelion/server/lib/alter.js

Line 20 in 4584a8b

    
           // In theory none of the args should ever be promises. This is probably a waste.

is also applicable for reduce and there's not actually something to resolve.

Tested and didn't notice any breakages, LGTM.

shahzad31

Uptime Changes Looks Good !!

cjcenizal

Changes to Rollup telemetry collector LGTM, but this code is really the domain of @elastic/kibana-vis-editors since it's about visualizations that use rollups.

x-pack/plugins/rollup/server/collectors/helpers.ts

…int-no-async-foreach

x-pack/plugins/security_solution/server/lib/detection_engine/rules/delete_rules.ts

FrankHassanabad

LGTM,

Looked over 1 section of code, left one comment
Talked about it with the internal team.

kibanamachine · 2021-09-14T18:31:20Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`maps`	3.2MB	3.2MB	+81.0B
`observability`	550.2KB	550.3KB	+101.0B
`security`	764.1KB	764.2KB	+114.0B
`visTypeVislib`	547.2KB	547.4KB	+106.0B
total			+402.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`kbnUiSharedDeps-elastic`	2.7MB	2.7MB	+11.0B
`kbnUiSharedDeps-js`	6.5MB	6.5MB	+2.3KB
`maps`	80.7KB	80.8KB	+66.0B
`observability`	59.0KB	59.1KB	+66.0B
`security`	84.3KB	84.3KB	+66.0B
`visTypeVislib`	32.0KB	32.0KB	+66.0B
total			+2.5KB

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`maps`	1173	1176	+3

History

💛 Build #153067 was flaky 6d08466
💛 Build #153043 was flaky f9d171b
💚 Build #152950 succeeded 4c94b18
💔 Build #152897 failed 73e42fb
💚 Build #152416 succeeded 8f3b502

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

thomasneirynck

maps/* changes lgtm

🙇 asyncMap, so much cleaner.

nchaulet

Fleet tests changes 🚀

mshustov

Ok for the core changes. I don't want to block this PR. The core team can refactor the tests in @kbn/std later.

vadimkibana

bfetch code changes LGTM.

kibanamachine · 2021-09-14T20:24:32Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 111637

Co-authored-by: spalger <spalger@users.noreply.github.com> # Conflicts: # x-pack/plugins/event_log/server/es/init.ts

…2178) * [eslint] add rule to forbid async forEach bodies (#111637) Co-authored-by: spalger <spalger@users.noreply.github.com> # Conflicts: # x-pack/plugins/event_log/server/es/init.ts * remove unnecessary async forEach Co-authored-by: spalger <spalger@users.noreply.github.com>

spalger force-pushed the implement/eslint-no-async-foreach branch from cb2684a to 9fb9c19 Compare September 9, 2021 15:28

spalger commented Sep 9, 2021

View reviewed changes

...ns/security/public/management/users/components/confirm_delete_users/confirm_delete_users.tsx Outdated Show resolved Hide resolved

spalger force-pushed the implement/eslint-no-async-foreach branch from 9fb9c19 to ee60f73 Compare September 9, 2021 15:34

spalger commented Sep 9, 2021

View reviewed changes

src/plugins/vis_type_timelion/server/lib/reduce.js Outdated Show resolved Hide resolved

spalger commented Sep 9, 2021

View reviewed changes

src/plugins/bfetch/server/plugin.ts Outdated Show resolved Hide resolved

spalger force-pushed the implement/eslint-no-async-foreach branch 3 times, most recently from 49e8f46 to 74adce4 Compare September 9, 2021 17:52

spalger added 2 commits September 9, 2021 11:45

[eslint] add rule to forbid async forEach bodies and implement iterat…

9fad060

…ion helpers

manually update violations

20495b0

spalger force-pushed the implement/eslint-no-async-foreach branch from 74adce4 to 20495b0 Compare September 9, 2021 18:45

spalger added 6 commits September 9, 2021 12:15

update eslint errors with correct package name

b49a185

forEach over empty value must resolve

caa608a

ensure that map functions also work with empty iterators

081e27c

avoid confusing jest test checks

11f3108

fix imports

7d07a1a

fix some more violations

8f3b502

miltonhultgren mentioned this pull request Sep 13, 2021

[RAC] [Observability] Expand Observability alerts page functional tests #111297

Merged

spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Kibana-Operations Team v7.16.0 v8.0.0 labels Sep 13, 2021

spalger added 2 commits September 13, 2021 08:05

Merge branch 'master' of github.com:elastic/kibana into implement/esl…

73e42fb

…int-no-async-foreach

Merge branch 'master' of github.com:elastic/kibana into implement/esl…

4c94b18

…int-no-async-foreach

mshustov reviewed Sep 13, 2021

View reviewed changes

spalger added 2 commits September 13, 2021 14:03

Merge branch 'master' of github.com:elastic/kibana into implement/esl…

f206369

…int-no-async-foreach

use Array.from() instead of string.repeat()

f9d171b

spalger marked this pull request as ready for review September 13, 2021 23:36

spalger requested review from a team as code owners September 13, 2021 23:36

YulNaumenko approved these changes Sep 13, 2021

View reviewed changes

botelastic bot added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 13, 2021

move callback out of the asyncForEach

6d08466

spalger mentioned this pull request Sep 14, 2021

[eslint] add rule to forbid async subscribe bodies #112042

Closed

jportner approved these changes Sep 14, 2021

View reviewed changes

flash1293 approved these changes Sep 14, 2021

View reviewed changes

shahzad31 approved these changes Sep 14, 2021

View reviewed changes

cjcenizal approved these changes Sep 14, 2021

View reviewed changes

x-pack/plugins/rollup/server/collectors/helpers.ts Outdated Show resolved Hide resolved

cjcenizal requested a review from a team September 14, 2021 15:49

flash1293 reviewed Sep 14, 2021

View reviewed changes

x-pack/plugins/rollup/server/collectors/helpers.ts Show resolved Hide resolved

spalger added 2 commits September 14, 2021 08:56

Merge branch 'master' of github.com:elastic/kibana into implement/esl…

1f6be74

…int-no-async-foreach

remove unnecessary async

75024c6

FrankHassanabad reviewed Sep 14, 2021

View reviewed changes

x-pack/plugins/security_solution/server/lib/detection_engine/rules/delete_rules.ts Show resolved Hide resolved

FrankHassanabad approved these changes Sep 14, 2021

View reviewed changes

thomasneirynck approved these changes Sep 14, 2021

View reviewed changes

nchaulet approved these changes Sep 14, 2021

View reviewed changes

mshustov approved these changes Sep 14, 2021

View reviewed changes

vadimkibana approved these changes Sep 14, 2021

View reviewed changes

tylersmalley approved these changes Sep 14, 2021

View reviewed changes

spalger added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 14, 2021

spalger merged commit 2976f33 into elastic:master Sep 14, 2021

spalger deleted the implement/eslint-no-async-foreach branch September 14, 2021 20:20

spalger mentioned this pull request Sep 14, 2021

[7.x] [eslint] add rule to forbid async forEach bodies (#111637) #112178

Merged

mshustov mentioned this pull request Sep 17, 2021

Kibana crashes with ECONNRESET when connection to remote cluster is lost due to unhandled promises #110433

Closed

Conversation

spalger commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mshustov Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

spalger Sep 13, 2021 • 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!

mshustov Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

spalger Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

mshustov Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

spalger Sep 13, 2021 • 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!

mshustov Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

spalger Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YulNaumenko left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 13, 2021

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

shahzad31 left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FrankHassanabad left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 14, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

References to deprecated APIs

History

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

nchaulet left a comment

Choose a reason for hiding this comment

Uh oh!

mshustov left a comment

Choose a reason for hiding this comment

Uh oh!

vadimkibana left a comment

Choose a reason for hiding this comment

spalger commented Sep 8, 2021 •

edited

Loading

spalger Sep 13, 2021 •

edited

Loading

spalger Sep 13, 2021 •

edited

Loading