Skip to content

Commit

Permalink
chore: introduce promiseall-no-unbounded-parallelism linter rule
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rix0rrr committed Sep 30, 2024
1 parent 5e08c98 commit 50a07c9
Show file tree
Hide file tree
Showing 8 changed files with 73 additions and 1 deletion.
2 changes: 2 additions & 0 deletions tools/@aws-cdk/cdk-build-tools/config/eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down
8 changes: 7 additions & 1 deletion tools/@aws-cdk/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<Resource>` L1 resources come from the stable
* `invalid-cfn-imports`: Ensures that imports of `Cfn<Resource>` 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.
Expand All @@ -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
Expand All @@ -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 <https://ts-ast-viewer.com/> 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`.
1 change: 1 addition & 0 deletions tools/@aws-cdk/eslint-plugin/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
};
13 changes: 13 additions & 0 deletions tools/@aws-cdk/eslint-plugin/lib/private/type-checkers.ts
Original file line number Diff line number Diff line change
@@ -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';
}
Original file line number Diff line number Diff line change
@@ -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,
});
}
},
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure the number of awaited promises does not depend on program input
Original file line number Diff line number Diff line change
@@ -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([]);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
plugins: ['@aws-cdk'],
rules: {
'@aws-cdk/promiseall-no-unbounded-parallelism': [ 'error' ],
}
}

0 comments on commit 50a07c9

Please sign in to comment.