From 50a07c9ae95f3da2de1e4a765d43ff4b854a7a41 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 30 Sep 2024 11:18:57 +0200 Subject: [PATCH] chore: introduce `promiseall-no-unbounded-parallelism` linter rule Since JavaScript is single-threaded, `Promise.all()` will only ever be used for I/O-bound tasks. However, I/O-parallelism isn't free either. Every async I/O-performing task launched will consume some FDs, and their amount is limited. If the amount of promises launched is a function of the input the program runs on, the system FDs might be exhausted. Some concurrency limit must be introduced. This linter rule exists to remind the programmer of that fact. It triggers on every `Promise.all()` invocation and cannot be resolved; the only solution is to think about it, and then silence this rule as proof that you thought about it. --- .../cdk-build-tools/config/eslintrc.js | 2 + tools/@aws-cdk/eslint-plugin/README.md | 8 +++- tools/@aws-cdk/eslint-plugin/lib/index.ts | 1 + .../lib/private/type-checkers.ts | 13 +++++++ .../promiseall-no-unbounded-parallelism.ts | 39 +++++++++++++++++++ .../detect-promise-all.error.txt | 1 + .../detect-promise-all.ts | 4 ++ .../eslintrc.js | 6 +++ 8 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 tools/@aws-cdk/eslint-plugin/lib/private/type-checkers.ts create mode 100644 tools/@aws-cdk/eslint-plugin/lib/rules/promiseall-no-unbounded-parallelism.ts create mode 100644 tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.error.txt create mode 100644 tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.ts create mode 100644 tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/eslintrc.js diff --git a/tools/@aws-cdk/cdk-build-tools/config/eslintrc.js b/tools/@aws-cdk/cdk-build-tools/config/eslintrc.js index 31f822002716f..4f0ba2a313d60 100644 --- a/tools/@aws-cdk/cdk-build-tools/config/eslintrc.js +++ b/tools/@aws-cdk/cdk-build-tools/config/eslintrc.js @@ -45,6 +45,8 @@ module.exports = { '@aws-cdk/invalid-cfn-imports': ['error'], '@aws-cdk/no-literal-partition': ['error'], '@aws-cdk/no-invalid-path': [ 'error' ], + '@aws-cdk/promiseall-no-unbounded-parallelism': [ 'error' ], + // Require use of the `import { foo } from 'bar';` form instead of `import foo = require('bar');` '@typescript-eslint/no-require-imports': ['error'], '@typescript-eslint/indent': ['error', 2], diff --git a/tools/@aws-cdk/eslint-plugin/README.md b/tools/@aws-cdk/eslint-plugin/README.md index 65c28cc8d0f1f..09db7db88b325 100644 --- a/tools/@aws-cdk/eslint-plugin/README.md +++ b/tools/@aws-cdk/eslint-plugin/README.md @@ -4,7 +4,7 @@ Eslint plugin for the CDK repository. Contains rules that need to be applied spe ## Rules -* `invalid-cfn-imports`: Ensures that imports of `Cfn` L1 resources come from the stable +* `invalid-cfn-imports`: Ensures that imports of `Cfn` L1 resources come from the stable `aws-cdk-lib` package and not the alpha packages. Rule only applies to alpha modules. * `no-core-construct`: Forbid the use of `Construct` and `IConstruct` from the "@aws-cdk/core" module. @@ -17,6 +17,8 @@ Eslint plugin for the CDK repository. Contains rules that need to be applied spe * `no-literal-partition`: Forbids the use of literal partitions (usually `aws`). Instead, use `Aws.PARTITION` to ensure that the code works for other partitions too. +* `consider-promise-all`: when using `Promise.all()`, attest that there is no unbounded parallelism. + ## How to add new rules * Make a new file in `lib/rules`. It should export one function called `create`. The @@ -29,4 +31,8 @@ Eslint plugin for the CDK repository. Contains rules that need to be applied spe as well! * You can now run the test in debugging mode (make sure to have `npx tsc -w` running, then from a debugging terminal, `npx jest --no-coverage -it 'your rule name'`), set a breakpoint, and inspect the typeless objects. +Use to get a feel for the AST you're trying to analyze. Note +that eslint uses `estree` to model AST nodes (not the TypeScript AST nodes), but they are +often comparable. + To activate it for real on the repo, also add it to `cdk-build-tools/config/eslintrc.js`. diff --git a/tools/@aws-cdk/eslint-plugin/lib/index.ts b/tools/@aws-cdk/eslint-plugin/lib/index.ts index 834c6bf781d7d..26121b7e28603 100644 --- a/tools/@aws-cdk/eslint-plugin/lib/index.ts +++ b/tools/@aws-cdk/eslint-plugin/lib/index.ts @@ -3,4 +3,5 @@ export const rules = { 'invalid-cfn-imports': require('./rules/invalid-cfn-imports'), 'no-literal-partition': require('./rules/no-literal-partition'), 'no-invalid-path': require('./rules/no-invalid-path'), + 'promiseall-no-unbounded-parallelism': require('./rules/promiseall-no-unbounded-parallelism'), }; diff --git a/tools/@aws-cdk/eslint-plugin/lib/private/type-checkers.ts b/tools/@aws-cdk/eslint-plugin/lib/private/type-checkers.ts new file mode 100644 index 0000000000000..4ade572e680ed --- /dev/null +++ b/tools/@aws-cdk/eslint-plugin/lib/private/type-checkers.ts @@ -0,0 +1,13 @@ +// ESTree is supposed to be used dynamically typed (from JavaScript) and +// `@types/estree` only adds interface declarations, not helper functions to +// type guard objects. Therefore, we make a bunch here. + +import * as estree from 'estree'; + +export function isMemberExpression(x: estree.BaseNode): x is estree.MemberExpression { + return x.type === 'MemberExpression'; +} + +export function isIdentifier(x: estree.BaseNode): x is estree.Identifier { + return x.type === 'Identifier'; +} \ No newline at end of file diff --git a/tools/@aws-cdk/eslint-plugin/lib/rules/promiseall-no-unbounded-parallelism.ts b/tools/@aws-cdk/eslint-plugin/lib/rules/promiseall-no-unbounded-parallelism.ts new file mode 100644 index 0000000000000..006164aaf5503 --- /dev/null +++ b/tools/@aws-cdk/eslint-plugin/lib/rules/promiseall-no-unbounded-parallelism.ts @@ -0,0 +1,39 @@ +import { Rule } from 'eslint'; +import { isIdentifier, isMemberExpression } from '../private/type-checkers'; + +/** + * Get the programmer to acknowledge that `Promise.all()` is potentially dangerous. + * + * Since JavaScript is single-threaded, `Promise.all()` will only ever be used for + * I/O-bound tasks. However, I/O-parallelism isn't free either. Every async I/O-performing + * task launched will consume some FDs, and their amount is limited. If the amount + * of promises launched is a function of the input the program runs on, the system + * FDs might be exhausted. Some concurrency limit must be introduced. + * + * This linter rule exists to remind the programmer of that fact. It triggers + * on every `Promise.all()` invocation and cannot be resolved; the only solution + * is to think about it, and then silence this rule as proof that you thought about it. + * + * In summary, it's fine if: + * + * - The arguments to `Promise.all()` is a fixed set of promises; OR + * - The arguments to `Promise.all()` is throttled by a mechanism like 'p-limit' or + * similar. + */ +export function create(context: Rule.RuleContext): Rule.NodeListener { + return { + CallExpression: node => { + if (isMemberExpression(node.callee) + && isIdentifier(node.callee.object) + && node.callee.object.name === 'Promise' + && isIdentifier(node.callee.property) + && node.callee.property.name === 'all') { + + context.report({ + message: 'Ensure the number of awaited promises does not depend on program input, or their parallelism is limited using something like \'p-limit\' or similar. Acknowledge this message using \'// eslint-disable-next-line @aws-cdk/promiseall-no-unbounded-parallelism\'', + node, + }); + } + }, + }; +} diff --git a/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.error.txt b/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.error.txt new file mode 100644 index 0000000000000..d333fab5afc30 --- /dev/null +++ b/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.error.txt @@ -0,0 +1 @@ +Ensure the number of awaited promises does not depend on program input \ No newline at end of file diff --git a/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.ts b/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.ts new file mode 100644 index 0000000000000..8dd6d5d0b857e --- /dev/null +++ b/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/detect-promise-all.ts @@ -0,0 +1,4 @@ +async function main() { + // This is trivial and shouldn't count... but it's the best we can find right now. + return Promise.all([]); +} \ No newline at end of file diff --git a/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/eslintrc.js b/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/eslintrc.js new file mode 100644 index 0000000000000..9a0d0b21ed5a2 --- /dev/null +++ b/tools/@aws-cdk/eslint-plugin/test/rules/fixtures/promiseall-no-unbounded-parallelism/eslintrc.js @@ -0,0 +1,6 @@ +module.exports = { + plugins: ['@aws-cdk'], + rules: { + '@aws-cdk/promiseall-no-unbounded-parallelism': [ 'error' ], + } +}