Skip to content

Add @typescript-eslint/no-floating-promises#181456

Merged
afharo merged 99 commits intoelastic:mainfrom
afharo:no-floating-promises
May 1, 2024
Merged

Add @typescript-eslint/no-floating-promises#181456
afharo merged 99 commits intoelastic:mainfrom
afharo:no-floating-promises

Conversation

@afharo
Copy link
Copy Markdown
Member

@afharo afharo commented Apr 23, 2024 •
edited
Loading

Summary

Enable the ESLint rule @typescript-eslint/no-floating-promises (docs). It won't allow leaving promises unhandled.

Unattended promises will now require to either be awaited, .catch or, as a very last resource if you are absolutely sure that it will never reject and want to leave it unhandled, void.

For now, we've only focussed on the server-side because unhandled promises can crash the server. In the future, we might explore extending the rule to browser code.

Note to reviewers/codeowners

🛎️🚨 Please, consider creating follow-up PRs to log the caught errors, or add retries to your unattended setup/installing promises.

Identified offenders
  • node scripts/eslint_with_types --project examples/response_stream/tsconfig.json
  • node scripts/eslint_with_types --project examples/search_examples/tsconfig.json
  • node scripts/eslint_with_types --project src/core/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/content_management/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/data_views/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/data/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/discover/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/files/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/kibana_usage_collection/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/maps_ems/tsconfig.json
  • node scripts/eslint_with_types --project src/plugins/navigation/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/packages/ml/response_stream/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/actions/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/aiops/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/asset_manager/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/cloud_defend/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/alerting/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/canvas/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/ecs_data_quality_dashboard/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/cases/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/cross_cluster_replication/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/cloud_security_posture/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/elastic_assistant/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/event_log/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/file_upload/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/license_api_guard/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/license_management/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/lists/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/notifications/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/maps/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/fleet/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/enterprise_search/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/monitoring/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/ml/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/apm_data_access/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/logs_shared/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/observability_ai_assistant/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/apm/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/infra/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/observability/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/profiling/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/slo/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/uptime/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/observability_solution/synthetics/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/rule_registry/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/reporting/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/osquery/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/screenshotting/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/search_notebooks/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/search_playground/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/security/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/serverless_search/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/security_solution_serverless/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/spaces/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/task_manager/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/stack_alerts/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/timelines/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/threat_intelligence/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/stack_connectors/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/upgrade_assistant/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/test/functional_cors/plugins/kibana_cors_test/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/transform/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/triggers_actions_ui/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/test/plugin_api_perf/plugins/task_manager_performance/tsconfig.json
  • node scripts/eslint_with_types --project x-pack/plugins/security_solution/tsconfig.json

Typical use cases found

Jest tests not awaiting expect(promise).resolves and expect(promise).rejects assertions

If not awaited, jest just finishes the test without validating the assertion. Proof of that #181847

Unattended cleanups

Cleaning up PIT (point-in-time) search finders or task-manager tasks is important, although, sometimes, we don't want to hold the main business logic while doing that. We're using unattended promises in those scenarios. And that's fine! As long as those promises are caught.

Setup async operations

Kibana architecture does not allow plugins to use async lifecycle methods. This means that a plugin creating an index during setup/start can't await the promise, and they are sometimes left unattended.

We need to make sure that we catch those promises and either log the errors or attempt retries (if the setup really needs to happen).

core.getStartServices()

Holding some setup methods until it can access the start contracts.

public setup(core, deps) {
  core.getStartServices().then(([coreStart]) => {
    deps.someCoolPlugin.registerMyExtension(
      myExtension({ core: coreStart })
    );
  });
}

In general, you should reconsider your approach to a getter-based mode:

public setup(core, deps) {
  deps.someCoolPlugin.registerMyExtension(
    myExtension({ 
      getCoreStart: async () => {
        const [coreStart] = await core.getStartServices();
        return coreStart;
      },
    })
  );
}

If you can't, you can consider either using void or, if you think your code inside the .then might fail due to side effects, then make sure to add a .catch at the end.

Loops not returning the promise

This looks like an easy to miss overlook: looping to an array, and creating promises, but not awaiting them.

Doing this:

await Promise.all(myArray.map(
  (item) => {
    doSomethingAsync(item); // <--- 🔴 The promise is not returned in this array
  }
));

Instead of this:

await Promise.all(myArray.map(
  (item) => doSomethingAsync(item)
));

Or this:

await Promise.all(myArray.map(
  (item) => {
    return doSomethingAsync(item)
  }
));

try-catching a non-awaited promise

Some other cases, we correctly try-catch the promise, but forget to await it. This means the try-catch will only catch synchronous errors.

Instead of this:

try {
  doSomethingAsync(); // <--- 🔴 the promise is not awaited, so rejections are not caught
} catch (err) {
  // Handling error
}

Do this:

try {
  await doSomethingAsync();
} catch (err) {
  // Handling error
}

Risk Matrix

Risk Probability Severity Mitigation/Notes
The rule can be skipped using eslint-disable or simply adding void in front of the promise Low High We expect developers to use it with caution.

For maintainers

@afharo
Copy link
Copy Markdown
Member Author

afharo commented Apr 23, 2024

/ci

@afharo afharo force-pushed the no-floating-promises branch from 20d3bd5 to 5f98831 Compare April 23, 2024 16:30
@afharo
Copy link
Copy Markdown
Member Author

afharo commented Apr 23, 2024

/ci

1 similar comment
@afharo
Copy link
Copy Markdown
Member Author

afharo commented Apr 23, 2024

/ci

@afharo afharo self-assigned this Apr 23, 2024
@afharo afharo added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Team:Operations Kibana-Operations Team labels Apr 23, 2024
afharo
afharo commented Apr 24, 2024
View reviewed changes
@afharo
Copy link
Copy Markdown
Member Author

afharo commented Apr 24, 2024

/ci

2 similar comments
@afharo
Copy link
Copy Markdown
Member Author

afharo commented Apr 24, 2024

/ci

@afharo
Copy link
Copy Markdown
Member Author

afharo commented Apr 24, 2024

/ci

This was referenced Apr 26, 2024
Closed
Closed
Closed
Closed
mohamedhamed-ahmed
mohamedhamed-ahmed approved these changes Apr 30, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks for fixing this

@afharo afharo linked an issue Apr 30, 2024 that may be closed by this pull request
Kibana crashes with ECONNRESET when connection to remote cluster is lost due to unhandled promises #110433
Closed
@pmuellr pmuellr mentioned this pull request Apr 30, 2024
Closed
pmuellr
pmuellr approved these changes May 1, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the changes. This is going to be great!

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented May 1, 2024 •
edited
Loading

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
slo 63 62 -1

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5887 +5887
total size - 6.7MB +6.7MB
Unknown metric groups

API count

id before after diff
slo 63 62 -1

History

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

cc @afharo

@afharo afharo enabled auto-merge (squash) May 1, 2024 15:41
@afharo afharo merged commit 593d391 into elastic:main May 1, 2024
This was referenced May 1, 2024
Open
Open
@afharo afharo mentioned this pull request Jun 18, 2024
Closed
@afharo afharo mentioned this pull request Oct 15, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cnasikas cnasikas cnasikas left review comments

@ymao1 ymao1 ymao1 left review comments

@kpollich kpollich kpollich approved these changes

@nreese nreese nreese approved these changes

@stephmilovic stephmilovic stephmilovic approved these changes

@Dosant Dosant Dosant approved these changes

@sabarasaba sabarasaba sabarasaba approved these changes

@kdelemme kdelemme kdelemme approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@azasypkin azasypkin azasypkin approved these changes

@jbudz jbudz jbudz approved these changes

@hop-dev hop-dev hop-dev approved these changes

@mistic mistic mistic approved these changes

@seanrathier seanrathier seanrathier approved these changes

@sphilipse sphilipse sphilipse approved these changes

@lukasolson lukasolson lukasolson approved these changes

@mohamedhamed-ahmed mohamedhamed-ahmed mohamedhamed-ahmed approved these changes

@szaffarano szaffarano szaffarano approved these changes

@PhilippeOberti PhilippeOberti PhilippeOberti approved these changes

@semd semd semd approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@gergoabraham gergoabraham gergoabraham approved these changes

@marshallmain marshallmain marshallmain approved these changes

@cauemarcondes cauemarcondes cauemarcondes approved these changes

@pmuellr pmuellr pmuellr approved these changes

@ashokaditya ashokaditya Awaiting requested review from ashokaditya ashokaditya was automatically assigned from elastic/security-defend-workflows

@tiansivive tiansivive Awaiting requested review from tiansivive tiansivive was automatically assigned from elastic/security-entity-analytics

@rylnd rylnd Awaiting requested review from rylnd rylnd was automatically assigned from elastic/security-detection-engine

+2 more reviewers

@dgieselaar dgieselaar dgieselaar approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@afharo afharo

Labels

apm:review backport:skip This PR does not require backporting ci:project-deploy-observability Create an Observability project :ml rca-action release_note:skip Skip the PR/issue when compiling release notes Team:actionable-obs Formerly "obs-ux-management", responsible for SLO, o11y alerting, significant events, & synthetics. Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team:Detection Engine Security Solution Detection Engine Area Team:EnterpriseSearch Team:Fleet Team label for Observability Data Collection Fleet team Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// Team:ML Team label for ML (also use :ml) t// Team:Obs AI Assistant Observability AI Assistant Team:obs-knowledge DEPRECATED Former Obs Knowledge team. Team:obs-onboarding Observability Onboarding Team Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. Team:Operations Kibana-Operations Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Team:Security Generative AI Security Generative AI Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// Team:Threat Hunting Security Solution Threat Hunting Team Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// technical debt Improvement of the software architecture and operational architecture v8.15.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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