Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve all floating promise warnings #6000

Closed
turadg opened this issue Aug 19, 2022 · 5 comments · Fixed by #9458
Closed

resolve all floating promise warnings #6000

turadg opened this issue Aug 19, 2022 · 5 comments · Fixed by #9458
Assignees
Labels
code-style defensive correctness patterns; readability thru consistency hygiene Tidying up around the house vaults_triage DO NOT USE

Comments

@turadg
Copy link
Member

turadg commented Aug 19, 2022

What is the Problem Being Solved?

Since #5495 we have tooling that will detect and warn on floating promises.

4a84690 disabled them in CI as too slow. We've elected to postpone re-enabling them in CI until after launch. This is fine because we don't necessarily need the feedback on every run.

The problem at hand is that we have unhandled (floating) promises in master that may contain bugs. We've seen them hide bugs by creating bugs in tests.

Description of the Design

Before launch, handle all no-floating-promises warnings.

Tackle on a per-package basis by running,

NODE_OPTIONS='--max-old-space-size=8192' AGORIC_ESLINT_TYPES=true yarn lint:eslint

#5535 is an example. Though note it used scripts/lint-with-types.sh which since 4a84690 is a misnomer; it's more ci-lint.sh because it handles splitting the lint job and configuration of whether type linting is enabled in CI.

Security Considerations

--

Test Plan

Running AGORIC_ESLINT_TYPES=true yarn lint | grep @typescript-eslint/no-floating-promises has no output.

@turadg turadg added hygiene Tidying up around the house code-style defensive correctness patterns; readability thru consistency labels Aug 19, 2022
@rowgraus rowgraus added the vaults_triage DO NOT USE label Jan 3, 2023
@turadg
Copy link
Member Author

turadg commented Jan 28, 2023

More fodder for prioritizing this: https://gist.github.com/samsiegart/f74a97206d43cdb7c5095714f84136fc

@turadg turadg self-assigned this Jan 30, 2023
@ivanlei ivanlei added this to the Vaults RC0 milestone Feb 1, 2023
@ivanlei ivanlei assigned samsiegart and unassigned turadg Apr 19, 2023
@samsiegart samsiegart assigned ivanlei and unassigned samsiegart Apr 27, 2023
@samsiegart
Copy link
Contributor

PR#7485 will be merged soon.

Assigning back to @ivanlei to delegate remaining work

kriskowal added a commit that referenced this issue May 1, 2023
kriskowal added a commit that referenced this issue May 1, 2023
kriskowal added a commit that referenced this issue May 1, 2023
@kriskowal
Copy link
Member

I’ve sunk all floating promise warnings except those in:

  • zoe (118 cases, each a hard choice)
  • swingset (linter does not halt or report—presumably too big)

@turadg
Copy link
Member Author

turadg commented Aug 16, 2023

This will be closed when this report is empty:

git grep -rl no-floating-promises packages | sed -r 's/packages\/([[:alpha:]-]+)\/.*/\1/'  |sort |uniq

mergify bot added a commit that referenced this issue May 28, 2024
refs: #6000

## Description

I keep seeing these warnings and they're distracting. This burns down
almost all of them. What's left are a handful in Zoe that may be best
for @Chris-Hibbert to tackle directly.

### Security Considerations

none

### Scaling Considerations

none

### Documentation Considerations

none
### Testing Considerations

Some of these change the test. Reviewers should be confident in the
changes.

### Upgrade Considerations

None of these should change the end-user API. Some may fix bugs.
@turadg
Copy link
Member Author

turadg commented May 30, 2024

Since #9416 all that's left is the Zoe package. https://github.com/Agoric/agoric-sdk/compare/6000-no-zoe-floating-promises has some work towards it.

@mergify mergify bot closed this as completed in #9458 Jun 7, 2024
mergify bot added a commit that referenced this issue Jun 7, 2024
closes: #6000

## Description

Resolve remaining floating promises in Zoe

### Security Considerations

Zoe's correct functioning is crucial to the chain. I determined that it
was correct to not further handle any of these promises.

### Scaling Considerations

N/A

### Documentation Considerations

N/A

### Testing Considerations

N/A

### Upgrade Considerations

No impact, because no behavioral changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-style defensive correctness patterns; readability thru consistency hygiene Tidying up around the house vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants